extra action

1
2
3
4

  referral_info = { :user => current_user, :referer => referer }
  referral_info.merge!(:team => Team.find(session[:team_id])) unless session[:team_id].blank?
  Referral.create(referral_info)

It’s a really common pattern for me, in fact. Thoughts?

— Morgan

I have found the following to be extremely useful to replace the conditional logic in your example above:

1
2
3
4
5
6
7
8
9
10

class Object
  ##
  #   @person ? @person.name : nil
  # vs
  #   @person.try(:name)
  def try(method)
    send method if respond_to? method
  end
end

from:

http://ozmm.org/posts/try.html

Wouldn’t it be easier to perhaps move this logic into the model instead of having it in the controller? You could assign the team of the user who created the referral on the before_validation_on_create callback if one is available.

Then your controller would just be:

1
2

current_user.referrals.create(:referer => referer) 

The second version is the best. Jared, you just use ‘hack’ to mean, ‘code that ain’t my style’, but find_by_x was designed to work exactly as it does, and is considered natural by some (including me) because it fits the real-world logic of ‘getting something’ much better than an error. When you look for something that isn’t there, you don’t freak out, you just deal with the fact you don’t have it.

I have a feeling that your refusal to acknowledge this (or only as a hack) has to do with your apparent fundamental disagreement with the way nil is handled by ruby.

I like Morgan’s solution, and I use that same pattern an awful lot myself.

Fwiw, although I agree that find_by_id may be a bit of a hack (I’ve never liked the fact that a failing find will raise but find_by_id happily returns nil), I have to admit that in this scenario, it makes for much cleaner, more succinct code.

Ultimately, like most things, it’s a matter of style - which seems cleanest to you - because you’re the one who’s going to have to live with it, and defend it, when other people call bullshit.

Am I the only one that would use the following. Obviously it means that your controller has to know the team is related by using the field team_id but that is a fairly decent assumption.

1
2

Referral.create :user => current_user, :referer => referer, :team_id => params[:team_id])

Greetings,

@Eric: Presuming you mean session[:team_id], that probably works, and cuts a database load out of the picture…

It’s just that it bypasses the model’s associations, which is also often considered a little hackish, and not in the good sense.

I freely admit, I’m sometimes anal in that I don’t actually like my controllers to refer to my model’s associations by their specific id. I find it makes it easier to refactor later if I make them hand around objects, even if those objects are looked up by their own primary key.

That’s probably entirely my personal design sense, though.

— Morgan

I agree with Floyd on this one. Comparing find_by_x and inline rescues is somewhat misleading. Inline rescues are frowned upon because they can’t focus on a single exception, but must cover all runtime errors. This can hide actual bugs in the code. The behavior of find_by_x returning nil if the record isn’t found is completely consistent with the rest of rails. In fact, the behavior of find(id) raising an exception is the outlier. Perhaps find(id) should have been named find!(id).

If there’s a team_id, you should be able to call create on ‘Team.find(:team_id).referrals’ to get the association. Otherwise, you can just call it on Referral.

So, maybe use a conditional to figure out what should create the Referral, and then call create on that?

1
2
3
4
5
6
7
8
9

<pre><code>
unless[:team_id].blank?
  creator = Team.find(:team_id).referrals
else
  creator = Referral
end
creator.create(:user => current_user,
                        :referer => referer)
</code></pre>

I’m with Tammer and Floyd here. The comparison between find_by_id and inline rescues is kinda far fetched. I immediately wanted to suggest #try, but it appears someone else has. IMHO your example is a prime candidate for a fat model situation. Push that logic down into your models and make those controller actions braindead.

Whilst I agree with Tammer and Floyd somewhat, I think the problem with example 2 is that it doesn’t express your intent so well – coming to that example fresh, it’s not clear if team can be nil – wheras the first example makes it clear that the team attribute is optional. As a result, I think that Morgan’s example (in the first comment) seems a good balance of succinctness and clarity of intent.

(I know we’re not talking about the domain model here, but man: Apache has broken people’s spelling. A Referral should surely have_one :referrer?)

I’m with jonnii on this one. this is an awful lot of model specific logic that youre trying to muck around with in the controller when really the team model is who should be concerned with the team_id being valid. Adding validation code to the controller has a certain “smell”.

Assuming that you are validating the referral at some point, all you need to do is pass it the id. If the Team doesn’t exist it should fail validation anyways. no need to use Team.find_by_id(session[:team_id]) or even Team.find(session[:team_id]) if your validation is going to enforce the data integrity with a query anyways on validation.

1
2

validates_associated :team, :if=>Proc.new{|r| !r.team_id.nil? }

1
2
3
4
5

  team = Team.find session[:team_id] unless session[:team_id].blank?
  Referral.create(:user => current_user,
                  :referer => referer,
                  :team => team)

@Randy:

I don’t like that version of Object#try. What happens when the object understands that message because of missing_method magic?

1
2
3
4
5
6
7
8
9
10

class Object
  ##
  #   @person.name unless @person.nil?
  # vs
  #   @person.try(:name)
  def try(method)
    self.send(method) unless self.nil?
  end
end

The last version works because variables are created at parse-time, so when the parser sees “team = ” it concludes that team is a local variable and sets it to nil. The variable isn’t changed at runtime when session[:team_id].blank? and remains nil.

Here’s my take on #find raising an error, returning nil or returning an empty array vs #find_by_id returning nil. I think #find should act like #find_by_id and return nil. This seems more consistent to me. There should be a new method: #find! that would raise RecordNotFound in all situations where records were not found.