[Refactor]Reserwavtion system - action book

[regedarek]

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

[Dave Aronson]

On Mon, Feb 6, 2012 at 13:16, regedarek <dariusz...@gmail.com> 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

[Dheeraj Kumar]

You should move most conditions to validations. That IMHO is the best way to refactor this.

Dheeraj Kumar

--

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 rubyonra...@googlegroups.com.

To unsubscribe from this group, send email to rubyonrails-ta...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.

[Rolando Garro]

delegating some logic in to models methods might help.

[Dave Aronson]

On Mon, Feb 6, 2012 at 13:53, Dheeraj Kumar <a.dheer...@gmail.com> 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".... ;-)