Refactoring view logic in Rails

Question

Here's what I need to do. I have a Tournament model, which is connected to User via Signup (N:N).

The only thing that Signup adds is status of the signup. Tournament has a start time, and users can register only until there is 60 minutes before the tournament starts. After that, registered users can check in. So basically I have two options for the state

In short, models looks like this

class Signup < ActiveRecord::Base
  REGISTERED = 0
  CHECKED = 1

  belongs_to :tournament
  belongs_to :user
end

class Tournament < ActiveRecord::Base
  has_many :signups
  has_many :users, :through => :signups
end

class User < ActiveRecord::Base  
  has_many :signups
  has_many :tournaments, :through => :signups
end

I skipped some code to keep this short. The problem is with the view, since I have a lot of conditions to keep in mind. Here's my actual code (using Slim as a templating engine)

- if logged_in?
  - if current_user.registered_for?(@tournament)
    - if @tournament.starts_at < 60.minutes.from_now
      p Signups are closed, only registered users can now check in
      - if current_user.registered_for?(@tournament)
        = button_to "Checkin!", { :controller => :signups, :action => :update, :id => @tournament.id }, :method => :put
    - else
      = button_to "Cancel your registration for the tournament", { :controller => :signups, :action => :destroy, :id => @tournament.id }, :method => :delete
  - elsif current_user.checked_in?(@tournament)
    p You have already checked in.            
  - elsif @tournament.starts_at > 60.minutes.from_now
    = button_to "Sign up for the tournament", :controller => :signups, :action => :create, :method => :post, :id => @tournament.id
  - else
    p
      | The tournament starts in less than 60 minutes, you can't sign in
- else
  p 
    | You need to 
    |  
    = link_to "log in", login_path
    |  to play

The problem is, I have no idea how to make this much cleaner. I mean yes I can add helpers for buttons, but that won't help me with the if if else else ugliness, because there are many different combinations. Here's a short list:

user isn't logged in
it's over 60 until the tournament starts and user hasn't yet registered for the tournament
it's over 60 until the tournament starts and user is already registered
it's under 60 minutes, but user isn't registered yet
it's under 60 minutes and user is registered but hasn't checked in
it's under 60 minutes and user already checked in

And this is just the tip of the iceberg, because admins should see more information than a regular user, but I don't want to complicate this question.

The main problem is, how should I handle cases like this? It just seems so terrible to do this in a view, but I don't see any other simpler way.

@coreyward yea, but that's not really helping with the "too many ifs" thing, it's only moving it to another place — Jakub Arnold
– Jakub Arnold, Commented Nov 23, 2011 at 19:59
I didn't actually submit that half-sentence comment, intentionally anyways. Ha! — coreyward
– coreyward, Commented Nov 23, 2011 at 20:53

apneadiving · Accepted Answer · 2011-11-23 20:17:49Z

5

A cleaner way would be to create meaningful methods on your models. For example, in your Tournament model, add something like :

def can_register?( user )
  !user.registered_for?(self) && self.starts_at > 60.minutes.from_now
end

And then in your view, you can check for can_register? before displaying something. Adding logic into the view like you did is not what is intended in a MVC application.

edited Nov 23, 2011 at 20:17

apneadiving

116k26 gold badges225 silver badges215 bronze badges

answered Nov 23, 2011 at 20:12

Dominic Goulet

8,1437 gold badges33 silver badges57 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Sean Hill Over a year ago

That's a good start. This with the addition of moving the view logic to helpers will make for a very clean, clear refactor.

iain · Accepted Answer · 2011-11-23 20:16:34Z

You should use an object to encapsulate the logic. Maybe something like this:

class UserSignup

  def initialize(user, tournament)
    @user, @tournament = user, tournament
  end

  def registered?
    @user.registered_for?(@tournament)
  end

  def signups_closed?
    @tournament.start_at < 1.hour.from_now
  end

  def checked_in?
    @user.checked_in?(@tournament)
  end

end

Which makes the view a lot simpler and doesn't require to much work. You'll see that a lot of duplication will be removed this way, and you can test your sign up logic independent of the view.

You could also make a presenter, which is a bit more involved, but cleans up your view even more. Have a look at gems like draper to help you with this.

class SignupPresenter

  def initialize(user_signup)
    @user_signup = user_signup
  end

  def register_button
    view.button_to("sign up") if @user_signup.registered?
  end

  # etc ...

end

Also, I would consider using different templates or even controllers for different users. So users that haven't signed in at all cannot even access this page and admins have a different controller (even namespace) all together.

I wouldn't just go in and split it into partials, because that would just hide the logic. I also like a separate object more than putting it into a model, because this way the model doesn't get cluttered as much and all the logic stays together, nicely focussed.

Rails3 (at least) you can now use rails presenters, they're built-in.

Collectives™ on Stack Overflow

Refactoring view logic in Rails

2 Answers 2

1 Comment

1 Comment

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

1 Comment

Related