Fix my mistakes. Just don't do it yourself.

When I arrived at my current job, most of the functionality from the Rails application I'm working on was already done by developers before me. There were still some loose ends to tighten, but almost everything was 'working'. I say 'working' in between single quotes, because while the application was certainly working, it really only worked if everything was done in a seemingly exact pattern. Do something different from the pattern, and something will break. Like "Click here and fill in this, in this exact order, and put your cursor there, or the app will give an error."

I understand why this was done this way. I usually code this way too: after deciding what I'm going to do, I whip up a very crude version of what I want to do, then I take care of some loose ends that could be made better. But there are some things I do along the way to avoid having too many loose ends when I finish writing my basic functionality.

Most of us have heard the phrase "premature optimization is the root of all evil" by Donald Knuth. While this is true, I believe that we should think from the beginning about optimization before writing code. Your algorithm or pseudocode should be as optimized as possible from the start to avoid these things. If you think through your problem well enough, too much optimization won't be needed.

With web development and frameworks such as Ruby On Rails, it's easy to forget these things. A lot of the times people just sit in front of their computers and code away, hacking up whatever's on their mind without thinking it through. I admit I've been guilty of this behavior before. But with age comes experience, and I've learned a lot since then.

Anyway, back to my current code. Apparently the developers working on it before just sat down and wrote a lot of code without thinking about such basic things, like error-handling. It's not like they didn't have time to do it. My workplace isn't a stressful place, my bosses are way nice, and while we do have deadlines, it's not like we have someone on our backs constantly nagging that we have impending deadlines. Since those original developers don't work here anymore, I can't ask them why they coded like this, so I have to just deal with it the best that I can.

I created a document in our company's Wiki with some coding tips that came off the top of my mind, so that hopefully any new hires can read it and follow them. Even if they know these things, it should serve as a reminder to follow these basic guidelines. By no means it's a complete document, but these are the most common mistakes I've encountered.

Always handle errors gracefully

Try to never show the user an error page. If something went wrong, show a message letting the user know what went wrong, preferibly a flash[:notice] or flash[:error] message, which will appear at the top of the page, and send them somewhere where they can either try again or get more information.

Bad Example (If the item for some reason doesn't exist, or a curious user changes the ID in the URL, it will show an not-so-helpful error page):

def offer_for_want
  @have_item = HaveItem.find(params[:id])
end

Good Example:

def offer_for_want
  begin
    @have_item = HaveItem.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    flash[:error] = "Some error"
    redirect_to :somewhere
  end
end

Use descriptive variables

Although using shorter variables keeps the functionality the same as using longer variable names, using descriptive variable names and conventions will help other developers understand how an action or function works more easily. Also, using normal Ruby and Rails variable naming conventions helps others get up to speed on the scope of the variable.

Bad Example (Notice the use of two- and three-letter variables, which are not descriptive to a developer who's just working on this portion of the code):

def update_relationship(hi, wi, opt, modify_opt = nil)
  _have_want = {}
  _have_want['HaveId'] = hi
  _have_want['WantId'] = wi
  _have_want['Delete_add'] = opt
  [...]
end

Good Example:

def update_relationship(have_item, want_item, update_type, modify_option = nil)
  have_want_hash = {}
  have_want_hash['HaveId'] = have_item
  have_want_hash['WantId'] = want_item
  have_want_hash['Delete_add'] = update_type
  [...]
end

Comment your code!

All controller actions and model functions should be commented before the action or function, giving a brief description on how it works. Also, if the code is not self-explanatory or is done in a non-conventional way, a brief explanation should be included as well to let other developers know why the code was written that way, or how it works. This will also be useful for generating code documentation with RDoc

Bad Example (No comments anywhere):

def rate( _user )
  @current_user = _user

  transaction do
    user_tv = view_for(_user)
    user_tv.next_step = View::CF[:next_step][:nothing]
    user_tv.status_for_user = View::CF[:status_for_user][:nothing]
    user_tv.rated = true
    user_tv.save!
    self.rate!
    self.reload
    return ended?
  end
end

Good Example:

# Action to rate a user after an action has been accepted, completed and shipped.
# Only the people involved in the action are permitted to rate each other.
def rate(_user)
  @current_user = _user
  # Reloading the object attributes from the database, in case something was changed during the rating process.
  self.reload

  transaction do
    user_tv = view_for(_user)
    # Sets the next step of the action to 'nothing' for both users involved,
    # indicating that the entire process is complete.
    user_tv.next_step = View::CF[:next_step][:nothing]
    user_tv.status_for_user = View::CF[:status_for_user][:nothing]
    # Set a flag to mark the user's action as rated, so that multiple feedback can't be given
    user_tv.rated = true
    user_tv.save!
    self.rate!
    self.reload
    return ended?
  end
end

Properly substitute variables from MySQL queries for security purposes

Any query to the database where the user can enter the parameters (for example, a user enters terms in a search field) can potentially allow a malicious user to run a harmful query against the database, like dropping tables. These user-entered parameters need to be protected from these types of problems.

Bad Example (In this example, a user can enter something like ; DROP TABLE users as their search parameters, deleting the users table):

@item = HaveItem.find(:all, :conditions => "title = #{params[:search]}")

Good Examples (All of these samples do the same thing, except Rails sanitizes the parameters to make sure no arbitrary SQL code is run):

@item = HaveItem.find_by_title(params[:search])

@item = HaveItem.find(:all, :conditions => [:title => params[:search]])

@item = HaveItem.find(:all, :conditions => ["title = ?", params[:search]])

Sanitize any user input before rendering it in the view

Anything the user enters should be treated as potentially hazardous. Any unsanitized output can be vulnerable to Cross-Site Scripting. Any fields in the view that show information entered by a user should be sanitized. It's as simple as adding the h function before the string.

Bad Example (Anything the user enters in the database, when shown later in a view, can execute arbitrary code):

<%= @item.description %>

Good Example:

<%= h @item.description %>

Keep presentation and business logic separate

Thanks to the MVC paradigm, Rails makes it easy for any new developer to get find presentation code or business logic, but only if they're stored in the correct places. Any business logic (such as declaring variables, conditionals, etc.) should be kept in the controller or model and not inside the views. Likewise, all presentation code (such as rendering page updates via AJAX) should be done in either regular view files or RJS templates, not in the controller.

Bad Example (Variables being declared inside the view):

<% result ||= match_result %>
<% size ||= :small %>
<% match = HaveItem.find(result.incoming_item) %>


  

    [...]
  

Good Examples (In both cases, this leaves the view with just HTML and some Ruby/Rails code inside):

Declare the variables in the controller action instead of the view

def action
  @result = Engine.results
  @match = HaveItem.find(@result.incoming_item)
  [...]
end

In case the view is a partial, declare the variables when calling the partial

<%= render :partial "match_results", :locals { :result => @result, :match => @match } %>

Of course, I'm not an expert on these things. Most of them are probably common sense to most people, but you'd be surprised how often these mistakes are actually made. I'm sure you all have seen these, or probably worse. Does anyone have any other tips that can be added? Feel free to leave a comment.