Refactoring legacy Ruby on Rails apps
I have this idea: Chathub, an app that turns Github's organizations and/or repositories into chat rooms, pretty much like Gitter. I use this idea every time I want to build a new PoC. So I did it in Rails, in Java when I wanted to go deeper into Spring Boot and two front end versions in Ember and React.
Well, thats a lot of things, I learned a lot as well and this week I wanted to polish the Rails app a little bit. There was (is, actually) a lot of room for improvement and I'm bringing up some of them.
Before, mostly of the logic was living in the controllers and some of them on the models. For example, the login controller:
def create store_github_token @user = User.find_or_create_from_auth_hash(auth_hash) @user.update_last_login UserService.new(@user, octokit_client).update_user_organizations session[:uid] = @user.uid MetricServices.user_signed_in(@user.uid) redirect_to rooms_path end
this method was responsible for:
- find or create an user based on github's response
- update his organizations in the app
- push a metric entry to database
A lot of things for one single method. The problem here is that the controller holds too much responsibilities. It knows how to create users, save metrics, update data and depends directly from implementations.
To change this, I like the idea of Commands: specialized classes that does one thing and does it well.
We can split this in at least three commands
This command is responsible for login the user (find or create based on Github's response) and return it:
class LoginUserCommand def call(auth_hash) user = User.find_or_create_from_auth_hash(auth_hash) user.update_last_login user end end
is responsible for update user's organizations
class UpdateUserOrganizationsCommand def call(octokit_client, user) organizations = octokit_client.orgs(user.nickname) organizations.each do |github_org| room = CreateRoomFromGithubCommand.new.call(octokit_client, github_org) user.rooms.push(room) unless user.rooms.include? room end end end
there is still room for improvement here, since
UpdateUserOrganizationsCommand knows about the existence of
CreateRoomFromGithubCommand. This dependency is not healthy. It may look like inoffensive but can lead to problems.
The last command in this chain is
class SaveMetricCommand def call(type, user, custom_data) Metric.create(type: type, at: DateTime.now, user: user, custom_data: custom_data) end end
pretty straight forward and self explanatory: it saves a metric indicator. It accepts an enum of predefined metrics.
The question is: how do we use it all this commands? I like the idea of Service Objects as a responsible to tie up commands (adapting the idea of Dawid). Some people treat commands as services but in the end, they provide the same output: small classes that are responsible for one thing only.
As a result, here is the LoginService:
class LoginService def call(octokit_client, auth_hash) user = LoginUserCommand.new.call(auth_hash) UpdateUserOrganizationsCommand.new.call(octokit_client, user) SaveMetricCommand.new.call(MetricTypes::USER_SIGN_IN, user, nil) user end end
It gets the output of one command, send to another (the user in this case), push the metric and still is only 8 lines long!
Now, lets take a look into our new login action:
def create store_github_token user = LoginService.new.call(octokit_client, auth_hash) session[:uid] = user.uid redirect_to rooms_path end
Clean, easy to test and maintain.
- Tests become easier. For example, to test
LoginUserCommandI only need to provide some fake Github's response with user data. I don't need to worry about the room update process throwing exceptions.
- Since command names are very explicit, onboard a new developer should be easy.
- Commands do one thing well (S from SOLID) and maintain this app is easier as there is only one place to touch code.
As I said, there is still a long way until I'm 100% proud of this PoC:
UpdateUserOrganizationsCommandis breaking Liskov substitution principle and Dependency inversion principle, since it depends directly from the concrete command and cannot be replaced with an interface
- Tests still needs to be done, since they were previously done for controllers
- There is no reason to use
websocket-railsor even the new
ActionCable. External services as Pusher, Firebase and similar can do this job better than a Rails app
- Metrics and messages can (should, actually) be sent async.
- Remove the logic from models
I'll continue work on it as a PoC for good practices, SOLID and OO, you can keep an eye on it on my Github