current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have fully working code, however I'd like to make it nice. I've got these three models:

1. User(can have both or either one)
  user can have one trucker
  user can have one owner_trucker

2. Trucker
  belongs to user
3. OwnerTrucker
  belongs to user

I'm not the only one working in the app, but it starts to annoy me how everyone is solving the issue on its way.

The problem is how to determine what the user is (a trucker or a owner_trucker or both). i.e if user.trucker is not nil, its a trucker etc.

So here are few examples from the application (various controllers), basically checking for the same thing but in a different way:

helper_method :profile_type_from_user
  def profile_type_from_user(user)
    return profile = user.trucker ? :trucker : :owner_trucker
  end

And then used in another controller like:

if profile_type_from_user(profile) == :trucker
else
end

Another example would be:

@profile = current_user.trucker ? current_user.trucker : current_user.owner_trucker

Checking if one or another:

current_user.trucker? && current_user.owner_trucker.nil?

It this refactorable?

share|improve this question
1  
Does the problem domain allow for a simplification of these roles? Are owner_truckers also a type of trucker? How is a user who is an owner_trucker different than if they were both a trucker and an owner_trucker? As it is seems like there are opportunities to remove these checks (or at least express them more meaningfully as part of the User) but without more understanding of the domain it's not clear what would be valid. –  Jonah Jan 15 at 3:13
add comment

1 Answer

up vote 1 down vote accepted

I think your suspicion that the code is a "wet" is correct. I would start moving these predicates into the model whenever they are:

  • Used more than once, or
  • Can make the code clearer by having a named predicate.

Consider replacing this:

helper_method :profile_type_from_user
  def profile_type_from_user(user)
    return profile = user.trucker ? :trucker : :owner_trucker
  end

With this:

class User
  ...
  def profile_type
    if trucker ? :trucker : :owner_trucker
  end
  ...
end

Similarly, consider replacing this:

@profile = current_user.trucker ? current_user.trucker : current_user.owner_trucker

with:

class User
  ...
  def profile
    trucker || owner_trucker
  end
  ...
end

@profile = current_user.profile
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.