Today we’re going through at the importance of naming things. Let’s dive into the next topic at Ezequiel’s blog post

Using Meaningless Names

Code reading, method/variable naming is difficult. It is an art that separates men from boys.

A friend once tweeted:

When you are working on a project that you are the only developer, it is easy to use nameless variables: a, b, c, x, y, z. Those are terrible variables names.

Look at this example:

class StudentHelper
  def monitor(a, b, c)
    if a.senior(c) && a.is_monitor(c) && !b.senior(c)
     link_to 'ask for help', ask_path(a)
    end
  end
end

This methods checks if a student ‘A’ is monitor for a course that student ‘B’ is doing. If so, display a link to contact student ‘A’. Take a look again to the code, is it easy to read? Is it clear? I don’t think so. How can we improve this code just renaming things?

The first step is renaming the helper method name and its variables

class StudentHelper
  def monitor?(student_a, student_b, course)
    if sutend_a.senior?(course) && student_a.monitor?(course) && !student_b.senior?(course)
       link_to 'ask for help', ask_path(student_a)
    end
  end
end

For keep renaming let’s extract a method:

class StudentHelper
  def monitor?(student_a, student_b, course)
    if student_a.monitor_at?(course) && student_b.coursing?(course)
      link_to 'ask for help', ask_path(student_a)
    end
  end
end

class Student
  def monitor_at?(course)
    self.senior?(course) && self.monitor?(course)
  end

  def coursing?(course)
    !self.senior?(course)
  end
end

We can keep renaming all day long. But it will became a big refactoring. For today it is fine to stop here. We just made simple to kids becomes men. Probably the final result will be something like that:

class StudentHelper
  def monitor?(student_a, student_b, course)
    if student_a.can_be_monitor_of(student_b, course)
      link_to 'ask for help', ask_path(student_a)
    end
  end

  or
  
  def monitor?(student_a, student_b, course)
    if student_a.monitor_of(course) && student_b.coursing?(course)
      link_to 'ask for help', ask_path(student_a)
    end
  end
end

The final message is: “Always be assertive”. If your method alerts you about lunch time than name it in a way that is easy to know its purpose. Today we saw that rename things properly can improve our code readability and makes our life easier. I think we got the right mindset here, in the next few posts we going to see another little things that can improve our life :smiley:

And remember, always leave things better than you found them!

See you soon.

The next topic at Ezequiel’s blog post is about logic inside the view layer. We will going through a common problem that can cause a lot of headaches to the maintainers.

Using Logic Inside The Views

Today I will bring to you guys another example from the real world. I am going to keep using the repository from the previous post.

The day was friday and a co-worker asked me for help. Our customer was needing to edit a field that was uneditable, but just admin users will be able to do that. She was trying to figure out what the hell her code does not worked.

The original code:

<div class="row">
  <%= f.select :unitweight_id, @unitweights.map {|r| [r.name,r.id]}, { :disabled => true} %>
</div>

Her editor had the following code:

<div class="row">
  <% if current_user.roles.first.id == 1 %>
    <%= f.select :unitweight_id, @unitweights.map {|r| [r.name,r.id]}, { :disabled => true} %>
  <% else %>
    <%= f.select :unitweight_id, @unitweights.map {|r| [r.name,r.id]} %>
  <% end %>
</div>

What’s the problem of doing that? It is just a simple “if/else” case. My co worker said: When the user’s first role isn’t equals 1 the select still disabled.

After some minutes we discovered a javascript at the bottom of the file:

<script type="text/javascript">
<% if @editing %>
  document.getElementById("aircraft_unitweight_id").disabled = true;
<% end %>
</script>

That’s not cool at all folks. Don’t do that!

Ok, we just remove the javascript and the problem was gone. But wait, what if we changed the database and the admin role id change? what if we needed to add another user that will be able to edit that field? But do not think in that right now.

I am talking about a big software, a complex one. Doing that kind of thing in our views can be very, very dangerous. Our first change is to remove the javascript. So, now our code looks like this:

<div class="row">
  <%= f.select :unitweight_id, @unitweights.map {|r| [r.name, r.id]}, {}, :disabled => (!current_user.roles.first.id.eql? 1) %>
</div>

Our code still works, but we can do better. We can use a helper method:

<div class="row">
  <%= f.select :unitweight_id, @unitweights.map {|r| [r.name, r.id]}, {}, :disabled => (disable?) %>
</div>
# Helper class
def disable?
  !current_user.is_admin?
end

# User class
def is_admin?
  roles.ids.first.eql? 1
end

With this simple change we doesn’t need to touch our view anymore. It’s not a fancy and sophisticated solution, but it’s a beginning. We can use things like presenters and decorators to solve these problems of logic inside views. But let’s keep simple for now. With simple things like that we solved the customer problem, and we are able to work outside the view in the logic part of our software.

we can celebrate our work, this was another big step for a cleaner and better software.

And remember, always leave things better than you found them!

See you soon.

Ezequiel Delpero recentily blogged about the Most Common Mistakes On Legacy Rails Apps. He made a list of common bad practices, and how to improve the code or avoid those bad practices. In the next few posts I am going to dive into each topic of his list.

Business Logic Outside Models

Let’s start by remember briefly what the ‘M’ of MVC is:

M = Model. It represents the business logic and holds the knowledge of the application. V = View. It represents the view ot the application, is how the model is represented to the final user. C = Controller. It is a link between the Model and View.

So, as we can see, the Model layer holds the business logic of the application. It’s his responsibility to handle with the logic.

There are some free candies that I like in this MVC approach:

  1. It’s easy to test the logic;
  2. If necessary, it’s easier to change the core language, because we don’t have to care if our controllers or views has the old logic;
  3. We don’t get married with any framework; and
  4. Specially if you are running a Ruby on Rails application, the test suite for the business layer, runs faster than never.

The following code is real. Extracted from a real world controller and it is up running in production. For some legal reasons I’ve changed the metaphor of it, but kept the logic. eg: car becomes airplane

We have an Hangar with a bunch of robots. Broken robots are flagged as an unavailable. Each unavailable has types and subtypes.

The above code was extracted from the TechnicalUnavailabilitiesController

def new
  @unavailability = Unavailability.new

  @robots = Robot.joins_data.where_data.order("[robots].prefix")
  @robots_map = Robot.joins_data.where_data
  @hang     = Hangar.select('hangar.id, hangar.name, hangar.prefix').find(:all)
  
  if params[:han_g].to_i != 0
    @han = true
    @hangar = Hangar.select('hangar.id, hangar.name, hangar.prefix').find(params[:han_g])
  else
    @han = false
    @hangar = Hangar.select('hangar.id, hangar.name, hangar.prefix').find(:all)
  end
  @roles = User.find(current_user.id).roles.first.name
  @unavailability_types = UnavailabilityType.where("id = 11 or id = 5 or id = 7")
  
  respond_to do |format|
    format.html # new.html.erb
    format.xml  { render :xml => @unavailability }
  end
end

We have a lot of work to do here, right? This is the first time ever that I saw that creepy code, I’ve just grabbed it from the source repository and changed the name of some models and variables names. This method has a lot of bad smells, but I’m going to focus in the business logic.

So, the first step is understand what the code are doing.

We instantiate a new Unavailability, found some robots and ordered them by prefix, than the first strange thing happens, we bring the robots again, but this time without ordering them. I don’t know why the programmer did that, but let’s keep looking at the code. The next step is bring all Hangars to the game and store them into @hang var. Now we check if the params[:han_g].to_i is different of zero, it means that we passed a id trough params. and we know what hangar we’ll work with, but if the params[:han_g].to_i is zero we bring all the hangars with @hangar variable.

Oh, Gosh. I can do at least 3 more articles with this method.

Back to work. Now our method picks the current user first role and find arbitrary Unavailability Types based on some ids. And our method finishes with a respond to.

Extracting logic from controller

I don’t care how crazy our logic is. The first step is extract the logic out of our controller. When I say extract is not re-hacktoring or even re-factoring, is just extract.

class Robot
  scope :by_prefix, -> { order("[robots].prefix") }
  # ...
end
class Hangar
  scope :basic_fields, -> { select('hangars.id, hangars.name, hangars.prefix') }

  def self.find_hangar(id)
    id = id.to_i
    if id != 0
      Hangar.basic_fields.find(id)
    else
      Hangar.basic_fields.all
    end
  end
  # ...
end
class UnavailabilityType
  scope :special_case, -> { where("id = 11 or id = 5 or id = 7") }
  # ...
end

Our controller’s method now looks like this:

def new
  @unavailability = Unavailability.new
  @robots = Robot.joins_data.where_data.by_prefix
  @robots_map = Robot.joins_data.where_data
  @hang = Hangar.basic_fields.all
  @hangar = Hangar.find_hangar(params[:way_p])
  @han = @hangar.is_a?(Array)

  @roles = User.find(current_user.id).roles.first.name
  @unavailability_types = UnavailabilityType.special_case
  
  respond_to do |format|
    format.html # new.html.erb
    format.xml  { render :xml => @unavailability }
  end
end

I’ve just extracted all the crazy logic out of the controller. In this point I still don’t care how crazy this code was/is, because now I can focus on fix things like:

@roles = User.find(current_user.id).roles.first.name

Now we are allowed to work over models logic and don’t need to care about controllers anymore, I rotate my efforts to make my business logic more clean, code more efficient and testable.

But this is another article. This method has a lot of problems, but for today we can celebrate our work, this was a big step for a cleaner and better software.

And remember, always leave things better than you found them!

See you soon.