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.
