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.

Commands

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:

1. find or create an user based on github's response
2. update his organizations in the app
3. 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

LoginUserCommand

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

UpdateUserOrganizationsCommand

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.

SaveMetricCommand

The last command in this chain is SaveMetricCommand.

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.

Services

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.

Benefits

• Tests become easier. For example, to test LoginUserCommand I 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.

Improvements

As I said, there is still a long way until I'm 100% proud of this PoC:

• UpdateUserOrganizationsCommand is 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-rails or 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