Confusion, Understanding And Improvement

For seasoned programmers, I imagine they can open up some code that is new to them and instantly spot things.  They may even have a chuckle, or make a comment about how the code is crap.  That isn't me.  I don't consider myself as seasoned, and I like to review code to learn from it.  However, I came across some code recently which caused me to pause and inside my head ask -"Why are they doing it this way?"  Was this a sign of my newbiness or that of my growth as a Ruby programmer?

Ok, time to take a look at the code that got my gerbil cage spinning:

#Ugly (original code)
def prepare_download
  @download_report_name = params[:report] or raise(Exception.new(:not_found), "No report specified.")
  @download_file_format = ( params[:format] ? params[:format].downcase : nil ) or raise(Exception.new(:not_found), 'No format specified.')
end

What caused me to pause was the code related to @download_file_format.  The first thing we see is a ternary operation that checks for params[:format].  If it exists, downcase it and assign it to @download_file_format.  If it doesn't exist, return nil.  Then we have that or which will raise an exception.  Why?  Why not return the downcase version of params[:format] if it exists, or raise an exception?  

So I thought the code would reader better if we used something like the following:

#Good (first pass of improvement)
def prepare_download
  @download_report_name = params[:report] or raise(Exception.new(:not_found), "No report specified.")

  if params[:format]
    @download_file_format = params[:format].downcase
  else
   raise(Exception.new(:not_found), 'No format specified')
  end
end

Since this was bugging me a little bit I decided to review with my pair.  I was happy to see his expression and agreement with me that the code could have been written better.  He took it another step and proposed the following:

#Best
def prepare_download
  @download_report_name = params[:report] or raise(Exception.new(:not_found), "No report specified.")

  raise(Exception.new(:not_found), 'No format specified') unless params[:format]
  @download_file_format = params[:format].downcase
end

Both iterations are better, and more readable version of the code.  The #Best example illustrates not only clean, concise code, but also shows how experience (by my pair/lead) really can shine through.  I will continue to strive for that level of experience.

Learn Something New Everyday

Last Edited by: John on February 11, 2016