CODEWITHSUNDEEP
ruby-on-railsruby
Spyros
Spyros

Reputation: 48706

Is this a good approach to DRY?

My controller action is like :

# check if user is already working
if current_user.working?
  flash[:error] = I18n.translate 'error.working'    
  redirect_to labor_url and return          
end

# check if user is doing a quest
if current_user.has_tavern_quest?
  flash[:error] = I18n.translate 'error.has_quest'      
  redirect_to labor_url and return          
end

Now, this is some code that seems to be repeating itself in other controllers and their actions as well. Therefore, i thought that it would be a good idea to DRY things a little. I thought of creating a user method like :

def is?(states)
    possible_states = :working, :doing_tavern_quest
    # check states
    # set flash messages, do the same things as above without the redirects
end

The idea is that i would now just use something like that in my actions :

redirect_to labor_url if current_user.is?(:working, :doing_tavern_quest)

Do you think that is a good idea ? Is it a good way to DRY things up or i could do it in a better manner ?

Upvotes: 1

Views: 102

Answers (2)

Dafydd Rees
Dafydd Rees

Reputation: 6983

Nothing wrong in principle with current_user.is?(state) except maybe the name (is_one_of? ...)

def is?(states)
    possible_states = [:working, :doing_tavern_quest] # note square brackets here
    # check states
    # set flash messages, do the same things as above without the redirects
end

You could extract method the redirect line and create a method in the ApplicationController class so that the code is even drier (if the redirection is to the same place.)

Upvotes: 1

DigitalRoss
DigitalRoss

Reputation: 146261

I like the and return pattern.

But unless there are lots of different dynamic conditions, why not encapsulate the User state within the model as much as possible?

Unless the controller needs to know details, how about:

redirect_to labor_url if current_user.busy? # no parameters

BTW, controller methods don't return anything, you could possibly do:

return redirect_to labor_url if current_user.busy?

Upvotes: 1

Related Questions