You might be right, I might be crazy...

...but the result is the same.

Recently while working on a feature request, I found a method needed to be changed due to the nature of the request.  We have a upload bid feature which allows the user to upload a csv file with their bids instead of navigating the view manually.  If there is an error, we display a message but the Submit button was still active.  We needed to disable this button when there were errors present during the file upload process.

Ok, let us take a look at the original code:

# original code
def disable_submit_button?
  !zero_quantity_bids? && all_bids_confirmed? && @file_upload_errors
end

The problem with the original code is that we check for file upload errors last and we are using the && operator, checking for the result on all three method calls.  To address the issue of disabling the Submit button if there were file upload errors, I made the following update:

# my solution
def disable_submit_button?
  return true if @file_upload_errors
  !zero_quantity_bids? && all_bids_confirmed?
end

While the solution above worked fine, and it read well to me, my team lead wanted me to try a different solution.  The result is the same, but take a look and the finalized version:

# finalized version (by team lead)
def disable_submit_button?
  @file_upload_errors || (!zero_quantity_bids? && all_bids_confirmed?)
end

The finalized version does reduce a line of code.  For me, my version reads better but I fully understand and can appreciate the finalized verison.  I would love to hear what your thoughts are?  Perhaps you would like to suggest another solution?

Learn Something New Everyday

Last Edited by: John on April 01, 2016