CODEWITHSUNDEEP
ruby-on-railsrubycoding-styleviewrefactoring
Jakub Arnold
Jakub Arnold

Reputation: 87220

Refactoring view logic in Rails

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:

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.

Upvotes: 1

Views: 1239

Answers (2)

Dominic Goulet
Dominic Goulet

Reputation: 8113

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.

Upvotes: 5

iain
iain

Reputation: 16274

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.

Upvotes: 2

Related Questions