Watching an excellent talk "Blending Functional and OO Programming in Ruby" given by Piotr Solnica on Full Stack Fest 2015 made me think. I have tried different ways of organizing my service objects and here is what I came up with.Background
The idea is to switch from Object Oriented mentality and stateful objects to more functional way of doing things when only data is passed through your system.
I am working on this motivational portal. Let's use a concept from the project for the sake of example. The models involved in this example are User, Motivation, Shopand Role.
Motivationis some data that defines Shopto which Userhas access to in a given month and his Rolein regards to that. Each month a Usercan be assigned to different Shopsand different Roles. Say, an employee this month and shop manager the next month. I call such managers "chief" since manager was already taken.Stuff everything into model
For current month we need to get the list of shops user can manage. And to do that we have to use multiple tables. The most obvious solution is to stuff everything into User model:class User < ActiveRecord::Base def chief_shops_for_month(month) shops.merge( motivations.for_month(month).where(role_id: Role.chief) ).uniq endend
Invocation is simple:shops = user.chief_shops_for_month(month)
Hmm that starts quite harmless. But let's measure the complexity with flog:10.0: flog total 10.0: flog/method average 10.0: User#chief_shops_for_month -:2 Flog
I use flogto check complexity of my classes and strive to keep the average under 5:$ flog app 1278.9: flog total 5.0: flog/method average29.7: User#none23.7: TrHelper#tr app/helpers/tr_helper.rb:221.0: Role#none... Service class
To me 10 is simply too much. We can break it down in two parts like this:class User def chief_shops_for_month(month) shops.merge(chief_motivation_for_month(month).uniq end private def chief_motivation_for_month(month) motivations.for_month(month).where(role_id: Role.chief) endend
But this is ridiculous. It adds a private method on Userthat is calling god knows what. Besides I don't like the 'Thin Controllers, Fat Models' concept. As a one-man operation there is simply not enough man-power to deal with 10 feet long models. My motto is:
I build service objects in a simple way. One public method callwith occasional parameters passed from controller (i.e. variables). Let's extract the query to a what my typical service object would look like:class User class ChiefShopsForMonth attr_reader :user attr_reader :month def initialize(user) @user = user end def call(month) @month = month user.shops.merge(motivations_this_month_user_was_chief).uniq end private def motivations_this_month_user_was_chief user.motivations.for_month(month).where(role_id: Role.chief) end endend
Let's keep the UserAPI unchanged so this is a true refactoring:class User def chief_shops_for_month(month) User::ChiefShopsForMonth.new(self).call endend
Let's see what flog has to say about that:18.5: flog total 4.6: flog/method average 7.8: User::ChiefShopsForMonth#motivations_this_month_user_was_chief -:17 6.7: User::ChiefShopsForMonth#call -:10
Okay this is better. Now let's see if we can improve it further and maybe make it more functional-ish.The talk
This is the talk Piotr Solicagave at Full-stack fest in 2015 that got me thinking about a better way of doing things.
The gist of it: Don't store state. Just pass everything to callmethod instead. That sounded interesting. Rolled up my sleeves and here we go.Refactoring Service object
Let's try with full head-on solution first. I would like to throw everything in the mix:Dependency injection Command-query separation Keyword arguments (Smalltalk-style) Intention-revealing method names CQS/CQRS Command-Query Responsibility Segregation
This is a simple principle that can be boiled down to this:
Don't modify and request data in the same method call
While getting here it became obvious to me that I didn't have to use user.motivationas mergeis taking care of this. And I think by relying on Motivationclass directly I can make it more intention-revealing, even if it adds another dependency.
New structure gains certain tidiness in the upper part. Now initializeonly does dependency-injection for the whole object, callsets up the parameters and returns selffor chaining. You don't have to return self, but it gives such nice flexibility so I don't see a point of not doing it. And finally you can get the list of shops for the given month by calling .shops.class User def chief_shops_for_month(month) User::ChiefShopsForMonth.new.call(user, month).shops endend
Things get hairy however after the privatemark. The state is now here in the and there are four attributes instead of two. Flog is not very happy about this either. Average complexity per method is somewhat lower, but the total is even higher.26.0: flog total 4.3: flog/method average 7.6: User::ChiefShopsForMonth#chief_this_month -:30 5.0: User::ChiefShopsForMonth#shops -:15 5.0: User::ChiefShopsForMonth#none Stateless Object
One thing I would like to discuss first. As Sandi Metz says:
You can break the rules if you have a good reason or your pair lets you.
Something along these lines at least.
So I claim that I don't need to inject dependenices for other Activerecord classes since they are not going to change. Thus I can get rid of initialize. Of course in more complex screnario this is not the best way to go, but this is a simple query object so for the sake of refactoring experiment I think this will pass.
I think the rule of thumb here is similar to what Piotr is suggesting:
Initialize with Service Objects you are depending on
Since I am not changing anything here apart from the inner state of the object and I am getting rid of the state, I don't need a separate method, thus my object shrink back it's API to the callmethod. Which is actually an improvement. Since now I can pass everything I need in one method call.class User class ChiefShopsForMonth def call(user:, month:) all_user_shops(user).merge(chief_this_month(month)).uniq end private def all_user_shops(user) user.shops end def chief_this_month(month) Motivation.for_month(month).where(role_id: Role.chief) end endend
As you can see state is not stored anywhere, okay the dependencies are not injected which is a questionable practice, but again I claim in this case this could pass.
The invocation is almost the same as with the previous example, the only difference is that we don't have to query for shops as this is a Query Object so it only returns result straight away. And API is effectively shrunk to a single class.class User def chief_shops_for_month(month) User::ChiefShopsForMonth.new.call(user: self, month: month) endend
Yes you have to call new, but only to instantiate an object. Or you can still inject dependencies for testing. As a side effect we can pass a hash as parameter. Piotr suggests to use value object. I think it is overkill here. I could easily pass a params hash if I wanted to, but I am afraid it might hurt readability. And now look at this:10.4: flog total 2.6: flog/method average 5.0: User::ChiefShopsForMonth#call -:4 3.4: User::ChiefShopsForMonth#chief_this_month -:14
The whole class is almost as simple as the original single method in User. It encapsulates everything it needs. It doesn't pollute models with methods that would never be used from other places. And has intention-revealing name and simple single-method API.Conclusion
These are just some ideas on how to approach refactoring of fat models and fat controllers. Please let me know if you think something could be improved.