The C42 guide to good code: 2 - Best Practices

These are the practices you should follow for robust and well maintained code.

General Practices

Single Object Responsibility

In object-oriented programming, the single responsibility principle states that every class should have a single responsibility, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility. The reason it is important to keep a class focused on a single concern is that it makes the class more robust.

Read more on c2

Well named methods and classes

"There are only two hard problems in Computer Science: cache invalidation and naming things.” - Phil Karlton.

The importance of well-named classes and methods can't be emphasized enough. When another developer (or you, a couple of weeks down the line) is reading the code, the names should make it obvious what the class / method is supposed to to. At the very least, keep away from cryptic abbreviations, as illustrated by Steven Deobald here.

Single assertion per test

Tests are easier to read if there is a single assertion, that tests a particular aspect of the functionality.

The above test can be split up into the following


Rails Best Practices

Skinny controller, fat model

Push all logic to the model.

In fact, there are standard patterns available for each of the 7 RESTful actions, so try to fit into one of them.

Use ! as a name for any method that saves in the database

By convention, ! is used for any ruby method is dangerous in some way. One dangerous effect that we often care about is that modifying an entity that will be persisted.

When to use this also fits in to transaction management, and these are decisions that should be made on a case by case basis.

But, for the most part, it's usually a good idea to name methods that do a save in the db as a side effect with a !, and use either exceptions or the errors collection to handle error scenarios.

Do not use RJS

RJS has the server return some javascript, which is evaluated on the client side. An example of code that will be returned by a POST create call may be as follows:

This is bad for multiple reasons:

  • Security implications of eval() on the client side
  • Debugging requires you to keep a lot of context about what state the dom is in. In the above example, it is important to know that the .story div is hidden
  • The API that we have written is pretty much tied to a particular page. If i'm calling my API from another screen, I will need to create another div with the same class (story)

Use handlebars to render AJAX calls

Maturity for AJAX calls is as follows:

  • Worst: Use RJS (see above)
  • Bad: Render HTML in the output of the method, and just attach that HTML to the DOM
  • Good: Let the API return some JSON. Use Mustache or underscore to render

Have a very RESTful API

REST is based on a PHd paper by Roy Fieldings.

Borrowing from REST, the rails world has evolved it's own set of best practices around how to build a URL. One thought is to have every URL be one of seven standard actions. Every modification of a resource can be modeled as a CRUD operation on another resource.

POST http://example.org/orders/14/cancel

can be remodeled as follows

POST http://example.org/orders/14/cancellation

In the latter case, we are creating a cancellation resource within an order. Internally, the service will cancel the order as the cancellation object is created. The cancellation object may or may not need to be actually stored in the database, or even modeled. The cancellation object is a facade to the external world, to help any HTTP aware client understand your interface.

C2 also has a nice article on REST

Make sure you return the correct error codes

A fundamental part of REST is to leverage HTTP status codes to describe the results of the recent operation.

Some important status codes that we use more often than others are

  • 200 => :ok
  • 201 => :created
  • 30x => redirect_to (url)
  • 404 => :not_found
  • 422 => :unprocessable_entity
  • 500 => this happens when the server crashes. Avoid this at all costs.

Also, see the full list of status codes.

Do not expose too much in a JSON/XML API

Remember that once you expose an API, you will have to honor it. Be very conservative, especially when exposing nested resources, and belongs_to relationships.

For most apps, you probably want to handle transactions at the controller level

For most CRUD operations, you probably want the entire operation to happen, or not happen. Transactions are something that should be studied on a case by case basis. However, for most cases, the easiest place to start and finish the transaction is at the controller level, so that your model is not littered with it.

Use settings logic to push configuration into files

Settings logic is a great way to have configuration outside of the application, and is a fantastic alternative to using global variables.

Especially when you are doing a feature toggle, or something else of the sort.

Not every model has to be database backed

One common anti pattern that is often observed is to have every single class inherit from ActiveRecord::Base.

This is clearly a no-no. Like with every other programming language, in ruby you will need classes to encapsulate various behavior that does not need to be persisted to the database.

You can include ActiveModel into any class to get some of the nice things you get from ActiveRecord, like to_json, etc...

Controller tests should include an assert on the status code

This will provide you with a helpful failure if your test fails due to a permission error / internal server error. Thus you readily know that the failure is due to something outside your actual assertion, rather than have to figure that out working backwards from a failure message related to your actual assertion.

The failure here doesn't provide a clue that the problem lies in the test setup. But when you assert on the expected status code:

Now you know the problem lies elsewhere.

/

The C42 guide to good code: 1 - Code Smells


This post is the first of a line of posts that are a guide to what we believe is good code. This first one is all about Code Smells. These are indicators that your code is hiding a deeper problem.


Law of Demeter

The formal definition for the law of demeter can be found on Wikipedia, but we will summarise to say that method calls in the following form are a really bad idea.

dog.legs.walk!

Instead, create a delegator in the parent class

class Dog
  def move!
    legs.walk!
  end
end

dog.move!

This allows Dog to control how it walks. If someday, we invent a robot dog that walks using a hover device, then we need only to change the implementation of walk!, that is internal to dog, and the external world need not do

dog.legs.walk! if dog.normal?
dog.hover_craft.hover! if dog.robot?

More formally:

Every method M on an object O should only access these types of methods:

  • methods on O itself
  • methods on M's parameters
  • methods on any objects created/instantiated within M
  • methods on O's component objects

Tell Don't Ask

When you want an object to perform an action, never ask the object for information about it's state, make a decision based on the information, and then tell the object to perform the appropriate action.

The responsibility of how to perform the action should reside in that object, not the caller. In short, tell the object what you want it to do, and let that object worry about how to do it.

Bad

if account.balance >= amount
  account.deduct(amount)
else
  raise NotEnoughBalance
end

class Account
  def deduct(amount)
    balance -= amount
  end
end

Good

account.deduct(amount)

class Account
  def deduct(amount)
    raise NotEnoughBalance unless balance >= amount
    balance -= amount
  end
end

Read further on c2.


Law of 7

If a method is over 7 lines of code, more often than not the method would be doing more than what it should, and a candidate for refactoring. At the very least, pull out a private method to improve readability of the large method.

Having a single level of indentation makes it obvious what every method does at a glance. For example:

def self.import_from_csv(csv)
  csv.each_line do |line|
    if line["type"] == "distributer"
      Document.new(line["foo"], line["retail_price"])
    else
      Document.new(line["foo"], line["wholesale_price"])
    end
  end
end

can be replaced with

def self.import_line(line)
  if line["type"] == "distributer"
    Document.new(line["foo"], line["retail_price"])
  else
    Document.new(line["foo"], line["wholesale_price"])
  end
end

def self.import_from_csv(csv)
  csv.each_line do |line|
    import_line(line)
  end
end


Polymorphism over if conditions and switch / case

Avoid if conditions & switch / cases whenever possible, instead preferring polymorphism over them.

Whenever possible, try to isolate the place where switch/cases are used to a single method where appropriate strategy (Strategy Pattern) is created to care of the behaviour, and allow polymorphism to take over.

def expiry_date
  case plan_type
  when 'subscription' then next_month
  when 'prepaid' then never
  end
end

def monthly_charge
  case plan_type
  when 'subscription' then 300
  when 'prepaid' then 0
  end
end

def charge_per_rental
  case plan_type
  when 'subscription' then 0
  when 'prepaid' then 100
  end
end

becomes

class SubscriptionPlan
  attributes :expiration_date => never, :monthly_charge => 300, :charge_per_rental => 0
end

class PrepaidPlan
  attributes :expiration_date => never, :monthly_charge => 300, :charge_per_rental => 0
end

def plan_type
  case plan_type
  when 'subscription' then SubscriptionPlan.new
  when 'prepaid' then PrepaidPlan.new
  end
end

def expiry_date
  plan_type.expiry_date
end

def monthly_charge
  plan_type.monthly_charge
end

def charge_per_rental
  plan_type.charge_per_rental
end


If you must use if conditions, then use it in one of the forms:

def sales_tax
  return 0 if purchaser.exempt_from_taxes?
  price * .10
end

def car.brake
  switch_on_brake_light
  unless stopped?
    slow_down
  end
end


Contributors:

  • Tejas Dinkar
  • Niranjan Paranjape
  • Smit Shah
  • Srihari Sriraman

Agile delivery needs design facilitators

With the growing awareness of the need to integrate user experience design into agile delivery, there is now a better recognition of the role played by design facilitators in enabling good design faster.  I wrote a blog post to share my insights on what should be a design facilitator's playbook:

A design facilitator's playbook

Would love to hear your thoughts and insights!

/

How would you describe RubyMonk's interface?

We receive a lot of love from our users about RubyMonk's visual design. And while we agree -- it's super cute! -- the site's visuals are hardly its interface. The "user interface" for RubyMonk is really the boundary between the student and the mentor.

How do we evaluate your code? How do we decide whether it is correct or not? These are the interesting questions in the user interaction. And, as emerges in the design of any product, these questions highlight the necessary unification of form and function, engineering and design, purpose and path. When you boil it down, the purpose of RubyMonk is to automate the teacher. On some level, this goal might seem akin to beating the Turing Test. In its current incarnation things are much simpler, of course.

I've explored this idea a bit in my post "defining the interface for rubymonk". Take a look and check back soon as I take this idea forward to discuss the yin and yang of tests and the boundaries they exercise.

/

Why you should respect deprecation warnings

At C42, I have worked on multiple Ruby on Rails migration projects. A simple task of upgrading an application from Rails 2.3 to Rails 3, can take anywhere from two days to three months. After such experiences, I have developed a small set of rules to make sure, my projects don't run into these two to three months long upgrades. A blog post by me on, how a simple discipline of fixing deprecation warnings can help you maintain your ruby applications in the long run.

/