Hi, I have in my controller action book, but it looks ugly. How to refactor its right way? def book> if params[:date].present? && params[:room].present? && current_user > room = Room.find_by_id(params[:room]) > date = DateTime.strptime(params[:date],"%d-%m-%Y-%H-%M") > unless date > Date.today+8.days || date.past? > unless Book.user(current_user.id).room(room.id).period(room.period).first > unless Book.room(room.id).free(date,date+room.duration.minutes).first > stop = (date+room.duration.minutes).strftime("%H:%M") == "00:00" ? "23:59" : (date+room.duration.minutes).strftime("%H:%M") > unless Book.room(room.id).cyclic.dayoftheweek(date.wday).onlytimefree(date.strftime("%H:%M"),stop).first > redirect_to list_books_path(:room=>room) > else > flash[:error] = "Obiekt #{room.name} is reserved each week" > end > else > flash[:error] = "Obiekt #{room.name} is not availible" > end > else > flash[:error] = "Obiekt #{room.name} can be reserved once on few days" > end > else > flash[:error] = "wrong date" > end > else > flash[:error] = "no raservation parameters" > end > end > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-talk/-/gbwpv_w55T8J. To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On Mon, Feb 6, 2012 at 13:16, regedarek <dariusz.finster-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:> How to refactor its right way?Without delving too deep into the actual logic.... You''ve got a bunch of "unless this else that". Generally speaking, if you''re using an "else", using "unless" makes it much more difficult for a reader to follow, because of the multiple negations. With an "else", stick to "if". Other than that, I''d suggest organizing it along the lines of: if some error condition complain about this one elsif another error condition complain about that one elsif some other error condition complain about the other one # lather, rinse, repeat else # all is happy! do what the user was trying to do end Now, within the "do what the user was trying to do", you may wind up finally being able to calculate or retrieve some things you need to analyze further error conditions. There are several approaches. You can just nest these again, within reason, or make the happy path a method call, wherein you repeat that pattern. -Dave -- Dave Aronson: Available Cleared Ruby on Rails Freelancer (NoVa/DC/Remote) -- see www.DaveAronson.com, and blogs at www.Codosaur.us, www.Dare2XL.com, www.RecruitingRants.com -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
You should move most conditions to validations. That IMHO is the best way to refactor this. Dheeraj Kumar On Tuesday 7 February 2012 at 12:19 AM, Dave Aronson wrote:> On Mon, Feb 6, 2012 at 13:16, regedarek <dariusz.finster-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org (mailto:dariusz.finster-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org)> wrote: > > > How to refactor its right way? > > Without delving too deep into the actual logic.... > > You''ve got a bunch of "unless this else that". Generally speaking, if > you''re using an "else", using "unless" makes it much more difficult > for a reader to follow, because of the multiple negations. With an > "else", stick to "if". > > Other than that, I''d suggest organizing it along the lines of: > > if some error condition > complain about this one > elsif another error condition > complain about that one > elsif some other error condition > complain about the other one > # lather, rinse, repeat > else # all is happy! > do what the user was trying to do > end > > Now, within the "do what the user was trying to do", you may wind up > finally being able to calculate or retrieve some things you need to > analyze further error conditions. There are several approaches. You > can just nest these again, within reason, or make the happy path a > method call, wherein you repeat that pattern. > > -Dave > > -- > Dave Aronson: Available Cleared Ruby on Rails Freelancer > (NoVa/DC/Remote) -- see www.DaveAronson.com (http://www.DaveAronson.com), and blogs at > www.Codosaur.us (http://www.Codosaur.us), www.Dare2XL.com (http://www.Dare2XL.com), www.RecruitingRants.com (http://www.RecruitingRants.com) > > -- > You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. > To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org (mailto:rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org). > To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org (mailto:rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org). > For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en. > >-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
delegating some logic in to models methods might help. On Feb 6, 12:16 pm, regedarek <dariusz.fins...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:> Hi, I have in my controller action book, but it looks ugly. How to refactor > its right way? > > def book > > > > > > > > > if params[:date].present? && params[:room].present? && current_user > > room = Room.find_by_id(params[:room]) > > date = DateTime.strptime(params[:date],"%d-%m-%Y-%H-%M") > > unless date > Date.today+8.days || date.past? > > unless Book.user(current_user.id).room(room.id).period(room.period).first > > unless Book.room(room.id).free(date,date+room.duration.minutes).first > > stop = (date+room.duration.minutes).strftime("%H:%M") == "00:00" ? "23:59" : (date+room.duration.minutes).strftime("%H:%M") > > unless Book.room(room.id).cyclic.dayoftheweek(date.wday).onlytimefree(date.strftime("%H:%M"),stop).first > > redirect_to list_books_path(:room=>room) > > else > > flash[:error] = "Obiekt #{room.name} is reserved each week" > > end > > else > > flash[:error] = "Obiekt #{room.name} is not availible" > > end > > else > > flash[:error] = "Obiekt #{room.name} can be reserved once on few days" > > end > > else > > flash[:error] = "wrong date" > > end > > else > > flash[:error] = "no raservation parameters" > > end > > end-- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On Mon, Feb 6, 2012 at 13:53, Dheeraj Kumar <a.dheeraj.kumar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:> You should move most conditions to validations. That IMHO is the best way to > refactor this.D''oh, you''re right, I totally glossed over that it was in his *controller*! Yeah, this is a canonical time when the Zen master should whack the student upside the head, and remind him of the mantra, "skinny controller, fat model".... ;-) -Dave -- Dave Aronson: Available Cleared Ruby on Rails Freelancer (NoVa/DC/Remote) -- see www.DaveAronson.com, and blogs at www.Codosaur.us, www.Dare2XL.com, www.RecruitingRants.com -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group. To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.