Wednesday, July 20, 2011

Yes, Nick - class methods are evil

My friend Nick has just posted an interesting article about the usage of class methods. The article is really good, go read it first and then come back.

Are class methods evil?

In my opinion class methods are almost always a wrong approach to a problem. It’s not wrong as in “it doesn’t work” but wrong as “we can make it better”.

ActiveRecord and Rails

I blame the ActiveRecord pattern for the current class methods problem. Every Rails application I see is full of globals which are often results or causes of class methods. The problem with ActiveRecord is that it forces you to think in terms of sql tables. Almost every table exists as a class with lots of class methods that let you access the data from this table.

I mean methods like:
User.find(id)
User.all
etc.

Can we do better?

One of the things Nick suggested is using user_factory. Overall he’s right, but I don’t think user_factory is a good name for that. We’re not producing users in our app, right? The users live in the context of our app. Is it a shop or a game or maybe a portal? Then create a class for that. This is your “factory”. Once you have it, always access users through this object. Not only users, you can do the same with products, prizes etc.

Instead of User.find(4) you now have game.users.find(4) or better game.find_user(4). You still gain from the ActiveRecord methods, without using the class methods, though.

How to implement the app class?

There are two ways that I’m aware of. Both are not ideal, mostly due to the way ActiveRecord works, but in my opinion they improve the code quality.

1. Create an ActiveRecord model for that.

If your app is a game then create a model called Game with the associated table (games). Yes, it will possibly have only one record and yes it may add some overhead to the queries that are generated by the ActiveRecord framework. Thanks to that however you may have only one “singleton/global” in your app.

class Game
  has_many :users
  has_many :prizes
  ....
end
I usually put the implementation in the ApplicationController and have something like:

def game
  Game.find_by_name(“Music Challenge”)
end

Then, in every other controller I can access the game object and find all the data I need through its scope.

2. Non-ActiveRecord class

The other way assumes that you hide the globals in this class. It can be done in many different ways. One of them is:

class Shop

  def products
    Product.all
  end

  def find_product(id)
    Product.find(id)
  end
end
What’s my choice?

Overall, I prefer the first way. It has a nice side-effect that once you have the game/shop model you can easily turn your application from a single game to a platform of games (which we actually did in one project, but that's another story).

I also try hard to find the way out of the ActiveRecord and sql/nosql dependencies and go into a pure OOP direction with madeleine/prevayler-like tools.

What’s your choice? Do you see the problem with class methods?


If you read this far you should Follow andrzejkrzywda on Twitter and subscribe to my RSS

18 comments:

Marcin Olichwirowicz said...

Let me put some of my thoughts here if you don't mind.

I completely don't get why your approach is better than using class methods. I'm trying to avoid using class methods as much as I can, but it does not look for me like a solution.

Second way it's just a running away and hiding class method invocation into wrapper object. It does not solve problem with avoiding class method calls.

First way is a bit better but...
game.find_user(4) is pretty similar to User.find(id). It does not changing my way of thinking to be more object-oriented than sql. It's just recall me a facade pattern.

What is more I think User.find(id) is much more flexible when you have more than 100 tables with a lot of relations, data, millions users. Every additional join is not desirable then and having one entry point like 'game' object could be a pain. Especially when you have to just get Pet.find_by_user(id).

But after all is a really cool discussion. Big kudos to both of you guys!

Bartosz Blimke said...

Class methods are sometimes misused but I wouldn't say they are evil.
Definitely not like static methods in Java were evil.

I've seen many times (and possibly even used sometime) class objects used as singletons and class methods used as singleton methods. Not perfect in terms of OOP, but not as painful either (as it was in Java :). Still testable and maintainable.
As long as a singleton object doesn't mix multiple responsibilities.

I'd say active record pattern is a different story and doesn't really represent the rest of OOP in ruby world. Maybe that's why some ruby programmers, having mostly experience with AR code, complain about class methods so much.
As you mentioned, in AR a class object represents a sql table, while and instance represents a row in that table.
This way, AR class fullfils at least responsibilities of a factory and of a collection. This often leads programmers to put all other methods related to managing objects of a class inside a class.
This end up with massive AR classes with dozens of methods.
Rails convention of "thin controllers, fat methods" multiplies this effect when (and actually very often) misunderstood.

This is often done by new programmers who started their OOP career with Rails, without having good OOP experience before.

If you consider evil as "in danger of being used by inexperienced OO programmer" to write evil code, then sure, it's evil :)

Andrzej Krzywda said...

Hey Marcin, thanks for your comment!

I agree, the second way is not really nice, however it doesn't have the performance impact the first one has.

Indeed, that's kind of a facade pattern here. I simply prefer the object-oriented thinking.

To me shop.products is clearer that Product.all and is worth the additional layer. It simplifies designing and thinking what should belong where - just think of it as modeling the domain with objects.

Bartosz Blimke said...

Nice examples 1 and 2 of trying to escape AR pattern to be more OO.
Although, fighting rails conventions usually ends up bad, maybe these techniques don't fight them as much. Too early to judge for me, without more experimenting with them.

Andrzej Krzywda said...

Bartosz:

"I'd say active record pattern is a different story and doesn't really represent the rest of OOP in ruby world."

When you work with Rails and ActiveRecord, then you have to use the ActiveRecord-style OOP which to me is just sql but with nicer DSL.

Just to be clear, I prefer ActiveRecord than using SQL directly. I'd love if there was a way of using better OOP with Rails, but the ideas presented in the post is the best I could find. I know it's not ideal.

I think that it also happens to more experienced developers that after a few years of ActiveRecord programming you're almost unable to do proper OOP, without globals, static methods etc.

Avdi Grimm said...

Yep, I agree. Class methods on Rails models are almost always an indication that someone has been thinking in SQL instead of in objects.

metaclass said...

classes is not "global". constants is global

Marcin Olichwirowicz said...

So from the other side... Which ORMs could you guys recommend as almost ideal in terms of pure OOP way of thinking? I was only dealing with PHP, Java, Python and Ruby ORMs.
Based on experience, in my opinion - Django has the most useful ORM with nice, object concept (but I didn't dive into the core so correct me if I'm wrong ;)). Samples of quering in Django - https://docs.djangoproject.com/en/dev/topics/db/queries/

Andrzej Krzywda said...

Marcin:

I never used Django, but from the link I can't see why it could be better. It's similar to ActiveRecord.

As for the real ORM, I haven't found anything really good in my career. I think that the problem is inherent to ORM, you just can't do super-nice OOP with relational databases behind your app.

I was most happy when I was using Prevayler with Java, and now when I experiment with Madeleine with Rails. Those are not real databases, you just keep objects in RAM and serialize them to the disk.

Once you stop using class methods then even ActiveRecord can give you some OOP background.

Alex Radhose said...

Actualy I like very much ActiveRecord approach. It's convenient to think of AR classes as database tables. It makes working with relational DB more transparent and OO.
Thanks for all your posts btw.

Marcin Olichwirowicz said...

For me Django looks more like this example Shop.products, you have a Product.objects.get(id=14). And with all those filters and so one it's a very nice approach for me. This objects methods are served by special Manager object, it recalls me something like EntityManager in JPA.
Madeleine sound quite interesting, I'm gonna try it soon - thanks for a tip ;)

Andrzej Krzywda said...

Marcin:

What's the difference between Product.objects.get(id=14) and Product.find(14). Is it the same?

The idea of a manager object sounds good. Need to read more about it.

As for madeleine, it's not very mature and stable, be careful and don't use it in production :)

Andrzej Krzywda said...

Alex:

"Actualy I like very much ActiveRecord approach. It's convenient to think of AR classes as database tables. It makes working with relational DB more transparent and OO."

Thanks for your comment. Yes, exactly, AR is perfect for working with relational DBs and make them a little more OOP.

In my opinion AR doesn't scale very well (in terms of the project size, not the speed). Its object orientation is limited and doesn't let you design you project easily. Things like DCI should help here, though.

Brian Durand said...

I don't see what is to be gained by trying to introduce a factory/builder layer into your app just to avoid calling a class method.

First of all, classes are objects so class methods really are instance methods on an object. There really isn't anything special about them other than that the classes themselves are singletons.

The way I like to look at it is that the User ActiveRecord class is the factory/builder for creating User objects and a pretty logical place to put the necessary code. Ultimately the code needs to live in some method somewhere.

Andrzej Krzywda said...

Test:

"I don't see what is to be gained by trying to introduce a factory/builder layer into your app just to avoid calling a class method."

It's a matter of personal approach to clean code, object orientation and readability. I prefer shop.products because it makes sense to me and because this is how I view the business domain - there are products in the shop.

Other people may prefer Product.all because they think of it in a more global way or because what they are modeling is not the business domain but the database below.

Similarly, I prefer shop.add_product as way of, well adding new product to the shop. You may prefer Product.create(..)

They way I was taught object oriented programming and the way I practice it is that when a noun appears very often in the conversation with the client or in the documentation then it should be a sign that there must be an object or class representing this concept.

When I work on e-commerce projects then the noun Shop appears very often. Makes sense to have a class from it. When I talked to the customers the concept of adding a product to the shop appeared quite frequently, so shop.add_product(..) makes sense to me.

I use OOP to help me represent the business domain in the source code. The better I do it, the less mistakes I may have.

Jeremy said...

I fail to see why you would want to create a manager object. You're adding overhead where it is just not needed. When looking at AR its better to think of the class methods as specialized constructors, you get an object, or collection of objects back. It is a clean way to mimic a repository pattern. Your suggestion really just adds extra cost to the system, at what real benefit?

Jan Dudulski said...

Although I like your point of view Andrzej, I also think as Jeremy, that AR class methods are "special constructors". What's the difference between User.new and User.find(id)?

Unless you always write something like Game.new_user, than ok - it makes sense :)

Anonymous said...

What's really wrong with class methods? I just see them as alternative initialization points, or an alternative to a separate Factory Class, they're not evil per se.

"OO-purity" is not really a perfect measurement when you're using a mixed-paradigm language.

I believe we should focus our energies on writing elegant implementations of class methods, not unelegant-but-by-the-books factories.