Refactor from scratch

working with legacy code on Ruby On Rails.

This week I have dedicated some time to one of those tasks that I always have in my mind but nobody never ask you to spend time until it is too late.

In production we have a controller that has been neglected and has become fat. Our app follows the MVC model and the ideal in these cases is to follow the rule of having controllers as few as possible and leave the full weight of logic within the model.

I must confess that, in the beginning the code of the controller was very simple and only was supposed to retrieve the conversations, but little by little, we added new tasks of what we call “easy ones” and the thing has gone wild.


First of all it is important to define what the controller is doing:

  • Check that the current user is not the admin and if it is, it returns an error message.
    • If it is not the admin:
    • checks if the params[:users] exists and if exists adds it as a parameter to the action that we use to get the conversation’s list.
    • Applies pagination to this list.
    • If the layout should be the new one and the parameter params[:new_layout] exists adds to the json response a new field named is_new_count with an integer corresponding to the number of new conversations.
    • In other case adds to the json response the result of c.patient.formated_answers.

The first step is to move all the actions inside the controller to external actions, in this case private methods of the controller, which I will call from the index.

My plan is that these actions will be responsible only for calling model methods.

At the time of testing I will start with the methods of the model and then checking that the actions of the controller call those methods.


In the user model I have defined this method:

There is a part of the code that handles patients_emails. This is part of a filter that is no longer used so it can be removed from the code leaving to the method the only task to check the role of the current user and apply the scope where_last_message_is_not_nil on the collection of conversations of this user.

This is the test code for the scope:

Y este el test para el método conversations_as_current_user:


The next method to be tested will be new_conversations_count that links the previous method with two scopes and returns the number of elements in the collection:

Eston son los dos scopes:

Y estos los tests para los dos scopes:

Once the scopes have been tested, the method test only has to ensure that the call to the 2 scopes is linked to the collection provided by conversations_as_current_user:



I move all the query logic to get the list of conversations according to the role of the user and the layout that uses to a private method outside of the main controller:

Y hago lo mismo con lógica que comprueba si estamos en la última pagina, consulta el numero de conversaciones nuevas y el proceso que da formato a la info relativa a las conversaciones que queremos enviar en nuestro json:

Leaving us with a controller much easier to read and that only takes care of calling different methods:

In the testing of the controllers we only have to worry about the index calling each of the methods but this is another story and as this post has been really long I will leave this question for another post.