Add shortcut methods or follow ActiveRecord semantics?

Question

Yet another argument with a friend of mine. Consider this code:

class User < ActiveRecord::Base
  has_many :groups
  def in_group?(group)
      groups.include?(group)
  end
end
class Group < ActiveRecord::Base
  has_many :members
  def add_user(user)
      members << user
  end
end

My opinion is that these methods add extra unnecessary complexity to the code and are hardly guessable — like why #in_group? but not #is_a_member_of?, or why #add_user but not #add_member, and so on. Coming from my 4 years experience with Rails and total 20 yrs programming experience, I'd better follow AR semantics and use User#groups.include?(group) and Group#members << user. They are easily guessable and in case if I'll need some extra functionality, I can use callbacks for has_many :members and override User#groups.include? in the association extension module if that would be necessary.

However friend of mine argues that it is better to use shortcuts to creates "points of abstractions" and it is better to extend this code rather than using callbacks or overloads.

What do YOU think?

P.S. just to be clear, I hate "WHAT IF" approach :)

Answer 1

I totally agree that these methods should not be added. It's not the job of models to create other related objects. That's the job of the association proxy. I'd also beware of overriding association proxy behaviors, since you're liable to see some strange side effects due to the way the proxies are implemented.

Answer 2

For in_group?: Your method name is guessable in one direction — it's pretty obvious what it does when you read it. This is the important kind of guessable. When somebody is maintaining this code, he won't get caught up on the in_group? call. It's also more declarative and concise, which improves readability (marginally in this case, but it's good as a rule). It's a Smalltalky kind of method.

On the other hand, I think the add_user method is unnecessary. It doesn't seem particularly more concise or direct, just nonstandard. The name is still fairly obvious and not egregiously harmful, but I don't see the benefit to it. It seems like more of a scenic route than a shortcut.

Answer 3

The style you quoted is defensible under the "Law of Demeter", which cautions against client code manipulating groups or members directly.

Answer 4

I am definitely in favor of abstraction, but not in this execution. Your friend raises a good point, What if how you determine if a member is in a group is going to change? You've got a ton of refactoring to do, good luck with that!

However, I see the argument of placing confusing functions in the wrong places. I'd say you've got an incorrect organization, why not attach them to the specific relationship model object?

example:

user.groups.in_group?("admin")

or

group.members.add_user(user)

That to me looks much more elegant, its a bit more verbose, but your separation is more relevant.

Answer 5

I'm in favor of these methods when there is cause (such as being a "member" in a group changes meaning). Having these methods encapsulates logic that will grow and change over time, and does aid in testing. Isn't this the whole point of OO programming?