Standards & Best Practices

Given that Subversion keeps an accurate record of everything that has ever been committed, there is absolutely no reason for code (or other assets) to exist in the project that is not currently being used. This means that whenever you come upon code that isn't currently being used, you should delete it (after, of course, you carefully confirm that it is not being used and you ensure that all of the tests pass after it's been deleted).

Every line of code has a purpose. When that purpose no longer exists, the code should disappear as well.

Do not use tab characters in any code files. Use spaces. There should be a setting in any IDE to convert tabs to spaces.

While we're at it, let's have everyone turn on your IDE's option to strip trailing spaces on save.

This way we won't keep getting whitespace changes in our diffs which makes looking at diffs more confusing and merging branches a LOT more difficult!

Though exceptions are a critical part of the Ruby programming language, there are many ways in which they can be abused. Whenever you're working with exceptions (or thinking about working with exceptions), keep these rules in mind:

You should never write a rescue clause without specifying which exception classes you're expecting to handle, and you should always be as specific as possible! This is because writing a blind rescue clause (one without a specified exception class, which defaults to RuntimeError) or writing a wide rescue clause (one specifying an overly broad exception class like Exception or StandardError) will indiscriminately swallow up every exception raised within that code block, making debugging problems related to that code (and sometimes problems far removed from that code) extremely difficult.

Do NOT do this:

def get_page
  begin
    Net::HTTP.get(URI.parse('http://www.example.com/index.html'))
  rescue Exception => e
    'There was an error.'
  end
end

Do this:

def get_page
  begin
    Net::HTTP.get(URI.parse('http://www.example.com/index.html'))
  rescue Errno::ECONNREFUSED
    'The connection was refused.'
  end
end

Remember, you can list more than one exception class as part of a rescue clause, you can have more than one rescue clause as part of a code block, and you can use ensure to run code after the block regardless of whether or not the block raised an exception.

An example:

def potentially_dangerous_method
  begin
    something_potentially_dangerous
  rescue ExceptionTypeA
    deal_with_exception_type_a
  rescue ExceptionTypeB, ExceptionTypeC
    deal_with_exception_types_b_and_c
  ensure
    always_do_something
  end
end

Exceptions should never be used as a substitute for simple conditional logic (if/else expressions, etc.) in specifying the program's flow-of-control! This is because the raising (and rescuing) of an exception is significantly less performant than a simple if/else expression.

Do NOT do this:

def percentage(part, whole)
  begin
    "#{part / whole * 100}%"
  rescue ZeroDivisionError
    'N/A'
  end
end

Do this:

def percentage(part, whole)
  if whole != 0
    "#{part / whole * 100}%"
  else
    'N/A'
  end
end

You should never let an exception disappear! Exceptions are to be used when something has gone wrong. When that occurs, recovering from the exception with a rescue clause is more helpful if you report the exception back to the team. One can use Hoptoad to aggregate all exception data. This is far more useful than writing the exception to a log file somewhere that nobody reads.

Do NOT do this:

def get_page
  begin
    Net::HTTP.get(URI.parse('http://www.example.com/index.html'))
  rescue Errno::ECONNREFUSED => e
    p e
    'The connection was refused.'
  end
end

Do this:

def get_page
  begin
    Net::HTTP.get(URI.parse('http://www.example.com/index.html'))
  rescue Errno::ECONNREFUSED => e
    notify_hoptoad e
    'The connection was refused.'
  end
end

There's a better way to organize environment initialization and specify gem dependencies. In the Rails::Initializer block of environment.rb, you can list out gem dependencies using config.gem …

See documentation here: http://ryandaigle.com/articles/2008/4/1/what-s-new-in-edge-rails-gem-dependencies

This system allows you to run rake gems to list required gems and rake gems:install to install required gems.

Also, gem-specific or plugin-specific configuration that needs to be done on environment initialization should be placed in .rb files under config/initializers rather than littering up environment.rb.

See documentation here: http://ryandaigle.com/articles/2007/2/23/what-s-new-in-edge-rails-stop-littering-your-evnrionment-rb-with-custom-initializations

The model layer should be where the real meat of the application resides. Whenever you're adding a new model to the application domain, there are a number of issues you have to address. Here they are, step by step:

The first thing to do when adding a new model is to write the model's migration. A few problems that have been noticed with migrations are:

• Forgetting to set column defaults (where appropriate)
• Forgetting to set column limits (where appropriate)
• Using confusing column names (especially regarding foreign keys)

Regarding foreign key columns, always name them after the model you're associating with (e.g. if an email address belongs_to a user, then the email_addresses table should have a user_id column). This is not the time to get creative with language for no good reason (e.g. by calling it owner_id instead). The only reason to ever not use only the model name is if you have multiple associations to that model and you need to differentiate the numerous relationships. It's a lot easier for developers to figure out what's going on if everything goes by the same name throughout the codebase.

The next step for adding a model is to create the class file within the app/models directory as well as the unit test file under the test/unit directory. This step is straightforward.

The absolute first thing that should be done within the new model file is the addition of the model's validations. They should be placed under a # Validations comment near the top of the model file (so add that comment if it's not there). It is critical that you take a moment to think about every single column you just created in your migration and decide what restrictions exist for the values of those columns. Consult the documentation for ActiveRecord::Validations::ClassMethods to see what validation macros are available to you.

A few key points:

• You might find that you need to go back and edit some column defaults/limits in your migration file. If this is the case, rake db:migrate:redo is your friend.
• If custom validations are required, do not define instance methods validate, validate_on_create, or validate_on_update. Instead, use the class methods of the same name and pass them symbols of your custom validation methods.
• Give your custom validation methods descriptive names and place the methods within the # Validations area of the model file rather than at the bottom. You can call private <method_name> just below the method definition if appropriate

The next issue that must be dealt with is one of security: marking certain model attributes as protected. Use ActiveRecord::Base.attr_protected to specify which attributes are protected from mass-assignment (see the Rails documentation to learn more). This step is critical in protecting data integrity from malicious users who tamper with URLs or forms.

The next most useful things to do with a new model is to setup the model's associations. This should be done under an # Associations comment near the top of the model file (so add that comment if it's not there). A few issues I've seen with associations are:

Forgetting to setup the reciprocal associations (e.g. if an email address belongs_to a user, then you'd expect a user to has_one email address) Setting up multiple, conditional associations to the same model when named scopes provide a better alternative (see Named Scopes)

The next step is to setup any callbacks that exist on your new model. Now that the associations are setup, you have to determine if creating/updating/destroying an instance of this new model should affect its associated objects. While destruction is typically handled by the :dependent ⇒ :destroy option on the association, creating and updating has to be done manually with callbacks. It's also a good idea to setup a validates_associated with the :on ⇒ :create option so you can validate any associated objects that are created automatically via callbacks (this way, the entire object tree that is created at once is known to be fully valid).

Whenever possible, combine multiple database migrations into one, especially when you're making multiple modifications to a single table. There's no reason to separate out individual column additions/removals/modifications into separate migrations.

Writing a script within a migration that uses application code ASSUMES that, when the migration is run, the application code will be the same as when the migration was written.

This is obviously a bad idea, so we should not be writing scripts that use application code within migrations.

Migrations should be used PRIMARILY to make modifications to the database SCHEMA. We should NOT use migrations to insert data into the database and instead use enumeration tables–which provide simpler management of fairly constant database records [like the 50 U.S. states, for example]). If a migration makes schema changes that require that records be updated, then there are two options to achieve the changes:

1. Use the utility methods within ActiveRecord::ConnectionAdapters::DatabaseStatements–like select, update, and insert–which give you access to raw SQL to make your changes. This, of course, goes around all of the application code, making it impossible to take advantage of all of the associations and helpful methods we have setup on each model. If the changes you need to make are fairly straightforward, this might be okay, making this an acceptable alternative.
2. If, however, writing out the raw SQL to achieve your changes makes your head want to explode (in other words, you really want to use existing application code), then you should put your script somewhere where the timing of its execution is not fixed (as it is within the world of numbered migrations). So, a rake task or just a file in /lib (which could then be run from script/console) would be fine. If you do that, you can update the script to your heart's content until it needs to actually be run, after which you can let it fall out of date and nobody cares because it never needs to be run again anyway.

Here's a great article on why named_scope is the best way to build conditions for database requests involving one or more models.

http://webjazz.blogspot.com/2008/08/named-scope-how-do-i-love-thee.html

As the article describes, this beats out setting up multiple associations as well as using association extensions when dealing with related models.

Our standard practice should be to setup only one association between related models, name it simply by the related model's name (pluralized in the case of has_many), and include no conditions upon the association itself. Leave that conditional work to named_scopes built on the related model.

So, here's an example of what NOT to do:

class Article < ActiveRecord::Base
  has_many :comments, :order => "comments.created_at desc"
  has_many :comments_with_email, 
           :conditions => "email is not null", 
           :order => "comments.created_at desc"
  has_many :comments_with_url, 
           :conditions => "url is not null",
           :order => "comments.created_at desc"
end
 
class Comment < ActiveRecord::Base
  belongs_to :article
end

and here's an example of what to do:

class Article < ActiveRecord::Base
  has_many :comments. :order => "comments.created_at desc"
end
 
class Comment < ActiveRecord::Base
  belongs_to :article
 
  named_scope :with_email, :conditions => "email is not null"
  named_scope :with_url, :conditions => "url is not null"
end

Controllers exist to do three things:

• To ask the model layer for data to be used in the view.
• To tell the model layer to save data submitted by users via forms and/or the query string (with no massaging of the data–let the model layer do that).
• To deliver the proper response to the user (redirect or render in some format).

That's it. If you see anything in the controller that doesn't directly support one of these goals, it doesn't belong in the controller.

Particular offenses to watch out for:

• Any validation/scrubbing of data on its way to the model layer (business logic).
• Any manipulation of associated models (business logic–see goal #2).
• Any query-building (model logic).
• Any view-building, be it HTML or Haml (view logic).

None of this stuff belongs in controllers.

A controller should only ever deal with one model, with very few exceptions (especially with Rails 2.3 multi-model forms). If creating a record should trigger a create/update of another record, that business logic belongs in the record's model's callbacks and not in the controller.

The 7 standard REST actions include:

1. index
2. show
3. new
4. create
5. edit
6. update
7. destroy

Acceptable additional actions are edit/update actions that only manipulate a subset of the record's attributes (rather than sending all of that through the basic update action and having to deal with branching the response logic), and show actions if we have multiple views of the same object.

All other methods on the controller that should not be publicly accessible as actions need to be listed under the private directive.

Follow this guideline from 20 Rails Development No-No's:

“Not counting respond_to blocks, shoot for controller actions of 5 lines or less.”

It usually isn't difficult to get a controller to 100% test coverage: just write a functional test that exercises each public action and ensures that no exceptions are raised. However, this really is not enough. Functional tests should cover the three tasks of controllers listed above and repeated here with guidelines:

1. To ask the model layer for data to be used in the view. Don't worry about checking the specific attributes of the loaded data (unit tests in the model layer are the appropriate place to confirm that a certain set of inputs returns a certain set of data), simply confirm that the right instance variables are populated by the right types of objects (or nil/empty where appropriate).
2. To tell the model layer to save data submitted by users via forms and/or the query string (with no massaging of the data–let the model layer do that). The success or failure of a create/update/destroy is technically a model layer concern, though it's okay to make basic assertions about them in functional tests–just don't be overly specific by checking that the parameters you put in actually made it into the database (leave that level of detail to the unit tests).
3. To deliver the proper response to the user (redirect or render in some format). This is usually the most substantial part of a functional test: making assertions about the response, any redirection, and any data stored in the flash, session, and/or cookies.

The names of view files should obey this format: action.format.renderer (e.g. show.html.haml NOT show.haml).

See the section labeled “Action Pack: Multiview” @ http://weblog.rubyonrails.org/2007/12/7/rails-2-0-it-s-done

This will become more important when providing data in formats other than HTML.

Call to_s on any Datetime object and pass a symbol of how you want it formatted. The available formats are in that conversions.rb. If the available formats do not serve the purpose add a time.rb in lib folder and add more formats in the same.

E.g of time.rb

class Time
  DATE_FORMATS.reverse_merge!(
    :abbreviated_date => '%b %d, %Y',
    :date_month_day => '%b %d',
    :date_fullmonth_day => '%B %d',
    :date_without_time => '%Y-%m-%d',
    :full_date_and_time => '%B %d, %Y %I:%M:%S %p',
    :full_slashed_date_and_time => '%m/%d/%Y %I:%M:%S %p',
    :slashed_date => '%m/%d/%Y',
    :time_with_seconds => '%I:%M:%S',
    :time_with_min_12 => '%I:%M %p',
    :slashed_date_with_hour_minute => '%m/%d/%Y %H:%M'
  )
end

Once defined the above formats can be used in the code along with to_s

E.g

Time.now.to_s(:slashed_date_with_hour_minute)

Always add any text that will be visible to the user in the translation file. All language translations should be in RAILS_ROOT/lang. The file is a simple YAML file which maps symbols to strings. Every language can map the symbols over to a different string, which can be changed based on user input. To translate symbols in a view, just call t(:symbol_name). To translate symbols in a controller or model call I18n.translate(:symbol_name).

Will add more about rspec later, as we plan to use rspec as the test framework.

Haml documentation can be found here: http://haml-lang.com/docs/yardoc/

Remember that, in haml, divs are assumed and that ids and classes are created with their css notation, for example:

#foo.bar

Becomes:

<div class="bar" id="foo"></div>

%span#woo.one.two

Becomes:

<span class="one two" id="woo"></span>

You only need to break into the hash-style options for classes or ids if they are dynamic, for example:

%div{:class => @user.username}

Becomes:

<div class="bob"></div>

In other words, we shouldn't be doing this:

%div{:id => "foo"}

When it's clearer to do:

#foo

Use Haml's various syntactical sugar elements. It's meant to be a beautifully clean way to write markup. If what you are writing isn't beautiful, it very well may be more complicated than it should be. An example of this is using ”=” versus ”==”.

Use ”=” when you want to directly write ruby code that gets its output sent to the view.

= User.count

Use ”==” when you want to send an escapable string to the view. If you could do the following

= "Hello there #{current_user.unique_name}"

you should instead do the simpler and cleaner version

== Hello there #{current_user.unique_name}

As you can see, the double equal sign immediately place you into the equivalent of a double quoted string.

Define all variables with a “var” in front of them, unless you want to define some global variable (using global variable itself has various issues so it is better to avoid it all together). While this isn’t necessary 100% of the time on 100% of browsers, it is necessary some of the time on some of the browsers, and it makes syntactical highlighting not freak out with warnings as well

Wrong:

element = $(element_id);

Right:

var element = $(element_id);

Make sure you are putting semicolons on the end of lines. Again this isn’t always needed all the time, but when it is needed, it takes a long time to find and figure out what is broken. This also helps when the javascript is minified and doesn't result into syntax errors.

Wrong:

elem.value = temp

Right:

elem.value = temp;

Try to keep spacing consistent as well. Put spaces between if and the parenthesis, between plus signs and whatever we are adding/concatenating.

Wrong:

if(contact.checked == true){
  names = names+symbol+contact.value;
}

Right:

if (contact.checked == true) {
  names = names + symbol + contact.value;
}

Try to reduce code when possible.

Wrong:

if(myfield.value.indexOf(".") == -1)
  return true;
else
  return false;

Right:

return myfield.value.indexOf(".") == -1

Finally, if you are writing javascript, please test it in a few browsers if possible.