Refactoring with Reek and RubyMine

I recently heard of a new gem called Reek. I decided to test it in a project of mine. Because I always wanted to thoroughly test RubyMine as well, why not use RubyMine to refactor the findings of Reek?

Let’s go!

Installing and using Reek

If you clicked the link above you saw the Github repo of Reek. There is all the information you need to use Reek in your own projects. But for completeness’ sake, let’s do the gem dance:

gem install reek

Using Reek:

reek app

This gave me a very long list of things that Reek found. 134 warnings overall.

crmapp master % reek app
app/admin/users.rb -- 9 warnings:
  [52, 52, 53, 56, 61]:create calls params[:user] 5 times (DuplicateMethodCall)
  [50]:create has approx 8 statements (TooManyStatements)
  [53]:create has the variable name 'p' (UncommunicativeVariableName)
  [77, 86]:update calls errors[:base] 2 times (DuplicateMethodCall)
  [84, 87]:update calls format.html 2 times (DuplicateMethodCall)
  [70, 71, 77]:update calls params[:user] 3 times (DuplicateMethodCall)
  [87]:update contains iterators nested 2 deep (NestedIterators)
  [66]:update has approx 13 statements (TooManyStatements)
  [71]:update has the variable name 'p' (UncommunicativeVariableName)
app/controllers/answers_controller.rb -- 1 warning:
  [1]:AnswersController has no descriptive comment (IrresponsibleModule)

# Lots of stuff omitted ...

app/models/dataset.rb -- 9 warnings:
  [34, 38]:Dataset has the variable name 'a' (UncommunicativeVariableName)
  [51, 51, 52]:Dataset#clear_times calls self.follow_up_time 3 times (DuplicateMethodCall)
  [60, 61]:Dataset#sanitized_time calls "%02d:00" % time_to_sanitize.to_i 2 times (DuplicateMethodCall)
  [60, 61]:Dataset#sanitized_time calls time_to_sanitize.to_i 2 times (DuplicateMethodCall)
  [56]:Dataset#sanitized_time doesn't depend on instance state (UtilityFunction)
  [56]:Dataset#sanitized_time has approx 9 statements (TooManyStatements)
  [63]:Dataset#sanitized_time has the variable name 'a' (UncommunicativeVariableName)
  [58]:Dataset#sanitized_time has the variable name 'i' (UncommunicativeVariableName)
  [56]:Dataset#sanitized_time refers to time_to_sanitize more than self (FeatureEnvy)

# Lots of stuff omitted ...

134 total warnings

As you can read in between, I omitted several lines. This is just supposed to be an example after all. I decided to show you three types of warnings:

  • An admin model app/admin/users.rb
  • A controller app/controllers/answers_controller.rb
  • An Active Record model app/models/dataset.rb (one God Object in this project...)

I will go through each one of these and try to reason about Reek’s warnings. I will use RubyMine to refactor parts of the code to get rid of the warnings and to have better code.

One more thought before I start. If you use tools like Reek, Brakeman or Flay and Flog please don't just follow their advice for the sake of following it. Always try to weigh on what you would like to achieve with your refactoring.
My thoughts for this projects are:

  • I often have multiple week-long breaks where I don't work on this code/project.
  • Everything I can refactor to make it easier for me to understand code I’ve written should help me.
  • I don’t have 100% test coverage for this project, so I can only refactor code that is covered by tests.
  • There are always exceptions to these “rules”

Refactoring the admin model

These were the warnings.

app/admin/users.rb -- 9 warnings:
  [52, 52, 53, 56, 61]:create calls params[:user] 5 times (DuplicateMethodCall)
  [50]:create has approx 8 statements (TooManyStatements)
  [53]:create has the variable name 'p' (UncommunicativeVariableName)
  [77, 86]:update calls errors[:base] 2 times (DuplicateMethodCall)
  [84, 87]:update calls format.html 2 times (DuplicateMethodCall)
  [70, 71, 77]:update calls params[:user] 3 times (DuplicateMethodCall)
  [87]:update contains iterators nested 2 deep (NestedIterators)
  [66]:update has approx 13 statements (TooManyStatements)
  [71]:update has the variable name 'p' (UncommunicativeVariableName)

Refactoring the #create action

I’ll start from the top. [52, 52, 53, 56, 61]:create calls params[:user] 5 times (DuplicateMethodCall) tells me that the create action calls params[:user] 5 times. The lines were it is called are given at the start of the warning.
Here’s the code for the create action:

def create # this is line 50
  generated_password = Devise.friendly_token.first(8)
  if params[:user][:invite_user].present? && params[:user][:password].blank?
    [:password,:password_confirmation,:current_password].collect{ |p| params[:user][p] = generated_password }
  end

  user = User.create(params[:user])
  if UserConfirmation.new.confirm_user(user.id)
    flash[:notice] = "Eine Einladung wurde an #{user.email} gesendet."
    redirect_to admin_users_path
  else
    flash[:error] = "Die E-Mail an #{params[:user][:email]} konnte nicht gesendet werden."
    redirect_to new_admin_user_path
  end
end

I will extract the calls to params[:user] as a variable. The screenshots show how to do this in RubyMine.

After calling the extraction you’ll have to name it. I decided to simply call it user_params.

The modified lines now look like this.

  # ...
  user_params = params[:user]
  if user_params[:invite_user].present? && user_params[:password].blank?
    [:password,:password_confirmation,:current_password].collect{ |p| user_params[p] = generated_password }
  end

  user = User.create(user_params)
  # ...

You probably already noticed the lines with the if statement. This is a perfect example for a good spot for refactoring. If you remember my goals from the beginning, there was this one:

Everything I can refactor to make it easier for me to understand code I’ve written should help me.

The if statement and the following line generates passwords and injects them into the user_params if there aren’t any. Why not take these lines and extract a method from them? I will name the method accordingly to what it does and everything will be clearer and neater.
While extracting the method I noticed, that I created a variable called generated_password at the beginning of the create action. This variable is only used with the injection method I just extracted. Instead of providing the variable as an argument to the method (as RubyMine suggested), I can move the whole line into the method. Then I provide the original params[:user] to the method, return the modified user_params from the method and assign them inside the create action to a local user_params variable.

What used to be four lines that didn’t reveal their intent unless you read every single one of them, is now one line with very clear intentions:

# sure, one could argue about the method name...
user_params = inject_generated_passwords_into(params[:user])

The controller action is now arguably better/clearer than before. The first two lines deal with creating the object. The rest is for rendering the appropriate response. There are people who don’t like explicit calls to render or redirect_to in their controllers. But once again I believe that they are perfect for revealing what happens and why does it happen. I prefer to have two or three more lines in my code, then having to scratch my head every time I see the code as to why things happen.

def create
  user_params = inject_generated_passwords_into(params[:user])
  user = User.create(user_params)

  if UserConfirmation.new.confirm_user(user.id)
    flash[:notice] = "Eine Einladung wurde an #{user.email} gesendet."
    redirect_to admin_users_path
  else
    flash[:error] = "Die E-Mail an #{user_params[:email]} konnte nicht gesendet werden."
    redirect_to new_admin_user_path
  end
end

By doing this we dealt with these three warnings:

  [52, 52, 53, 56, 61]:create calls params[:user] 5 times (DuplicateMethodCall)
  [50]:create has approx 8 statements (TooManyStatements)
  [53]:create has the variable name 'p' (UncommunicativeVariableName)

You may wonder what I did about the last warning. The answer is nothing. The warning is because of this line:

[:password, :password_confirmation, :current_password].collect { |p| user_params[p] = generated_password }

I will not change the p to somethings else, as this variable is only visible inside the collect block and it’s perfectly clear what it does. So I won’t change it, just because Reek doesn’t like it. Deal with it, Reek!

Refactoring the #update action

Once again, these were the warnings:

[77, 86]:update calls errors[:base] 2 times (DuplicateMethodCall)
[84, 87]:update calls format.html 2 times (DuplicateMethodCall)
[70, 71, 77]:update calls params[:user] 3 times (DuplicateMethodCall)
[87]:update contains iterators nested 2 deep (NestedIterators)
[66]:update has approx 13 statements (TooManyStatements)
[71]:update has the variable name 'p' (UncommunicativeVariableName)

And this is what #update looks like:

def update
    @user = User.find(params[:id])
  errors = {}

  if params[:user][:password].blank?
    [:password,:password_confirmation,:current_password].collect{|p| params[:user].delete(p) }
  # else
  #   errors[:base] = "The password you entered is incorrect" unless User.find_by_email(params[:user][:email]).valid_password?(params[:user][:current_password])
  end

  respond_to do |format|
    if errors[:base].blank? and @user.update_attributes(params[:user])
      if (@user == current_user)
        flash[:notice] = "Your account has been updated"
      else
        flash[:notice] = "The account #{@user.email} has been updated"
      end
      sign_in(:user, @user)
      format.html { redirect_to admin_users_path }
    else
      flash[:error] = "#{errors[:base]}"
      format.html { render :action => :edit, :status => :unprocessable_entity }
    end
  end
end

This looks bad, doesn’t it? Why hide it, it’s code I’ve written about a year ago. Things change and people get better. Now I have the chance to refactor it. Let’s see what can be done.

The first line, where I assign @user is indented 4 spaces. This is corrected very quickly.
In the next line I instantiate an empty hash and name it errors. I suppose this was done to have a place were I can shove any errors that occur during the saving of the user. The only problem with all this is: It doesn’t work!

I will delete everything but the neccessary parts of this method.

After:

def update
  @user = User.find(params[:id])

  respond_to do |format|
    if @user.update_attributes(params[:user])
      flash[:notice] = "The account #{@user.email} has been updated."
      format.html { redirect_to admin_users_path }
    else
      flash[:error] = "#{errors[:base]}"
      format.html { render action: :edit, status: :unprocessable_entity }
    end
  end
end

I use Active Admin for the admin backend. If something isn’t saved properly Active Admin takes care of notifying the user. I don’t have to do this myself. The whole “delete password if empty” dance was superfluous and didn’t work properly.
Hidden inside the successful if branch was a sign_in(:user, @user). This had the effect that if an admin updated another user account, the admin was logged out and imidiately signed in as this other user. HAVOC!

What a bad design.
If you look at the update action above, what do you see?

Right. Nothing special!. As I said, I use Active Admin. If the update action doesn’t do anything special it has no right to exist in the first place. Active Admin does all that by itself already. The best code refactoring is deleting code. So this is the update action after the complete refactoring:

It’s gone. Poof!

That means I’ve dealt with all warnings Reek gave me for the admin/users.rb file. Yeah! I rock. :-D

I’ve written in the beginning, that I want to deal with the warnings for two different files as well, but I won’t write about it this time. So please come back another time and read about the rest. And I will try to use RubyMine a bit more for the other files. In the end it wasn’t a fair competition for RubyMine. Every editor is the same when it comes to deleting code. So I can’t yet say wether RubyMine is good for refactoring.

If you don’t want to miss the second part of this post, you should subscribe to my newsletter. And you should probably follow me on Twitter. Although I post rarely over there.

Thank you for reading.