Do a Little Dance, Squash a Bug, Refactor-O

This week I had to work on a bug reported by QA.  Just a little background.  For this application, bidders can do a one time bulk upload of their bids.  We provided a comma delimited (CSV) file with all the important information.  All that is required is that one change the last value, which is the quantity, from 0 to the number desired.  They could leave a zero for all items they do not want to bid on.

During QA testing, it was discovered that if the upload file contained a value of 9.99 or even 9-Feb, the system would accept and process the bid with a quantity of 9.  However, instead of accepting the passed in value, which is a string when it comes in, we need to display an error message if the value isn't a positive integer.

Ok, let's take a look at the existing code to see what is going on.  Please keep in mind some of this has been modified a bit to protect the innocent, but is close enough.

# Original
def uploaded_quantity_valid?(quantity, item)
  return true if quantity == "0"

  unless /\d+/=~ quantity && quantity.to_i > 0
    context.error_msg "Invalid quantity (#{quantity}) for #{item}, must be a positive number."
    return false
  end

  if quantity.to_i > product.supply
    context.error_msg "Invalid quantity (#{quantity}) for #{item}."
    return false
  end
end

If our value in the upload file is 9.99 or 9-Feb, we do not see an error message and process the bid as a 9.  Why would we get a quantity of 9?  When we are processing the CSV file, the value for quantity, actually all values in the file, are strings.  When you do to_i on a string, Ruby does its best to make that value an integer.  Let's take a look by dropping into irb.

➜  ~  irb
2.2.3 :001 > quantity = "9.99"
 => "9.99"
2.2.3 :002 > quantity.to_i
 => 9
2.2.3 :003 > quantity = "9-Feb"
 => "9-Feb"
2.2.3 :004 > quantity.to_i
 => 9
2.2.3 :005 >

As you can see, to_i can be helpful, but it also can lead to problems.  So I had to rework the uploaded_quantity_valid? method to make sure we only process a positive number (integer). Let's see what we can do.  I'm not going to list all the things I tried, as there was some trial and error while noodling about in irb.  But what is below is what I ended up testing with and I'll call it the first iteration.

# First iteration
def uploaded_quantity_valid?(quantity, item)
  return true if quantity == "0"

  if quantity.match('\D')
    context.error_msg "Invalid quantity (#{quantity}) for #{item}, must be a positive number."
    return false
  end

  unless quantity.to_i > 0
    context.error_msg "Invalid quantity (#{quantity}) for #{item}, must be a positive number."
    return false
  end

  if quantity.to_i > product.supply
    context.error_msg "Invalid quantity (#{quantity}) for #{item}."
    return false
  end
end

This works, but as you can see we have some duplicate code.  Since this code passed my cucumber tests, I felt confident that I would refactor/re-work the method a bit to find a better solution.  My tests will save my butt if I screw up.  Below is what I ended up with, and I'll call it the final iteration:

# Final iteration
def uploaded_quantity_valid?(quantity, item)
  return true if quantity == "0"

  unless !quantity.match('\D') && quantity.to_i > 0
    context.error_msg "Invalid quantity (#{quantity}) for #{item}, must be a positive number."
    return false
  end

  if quantity.to_i > item.supply
    context.error_msg "Invalid quantity (#{quantity}) for #{item}."
    return false
  end
end

If you would like to try out this code on your own, fire up irb and use the following - note I made some minor changes so that the method will work as is in your irb session:

quantity = "9.99"
item = "bobots"


def uploaded_quantity_valid?(quantity, item)
  return true if quantity == "0"

  unless !quantity.match('\D') && quantity.to_i > 0
    puts "Invalid quantity (#{quantity}) for #{item}, must be a positive number."
    return false
  end

  if quantity.to_i > item.supply
    puts "Invalid quantity (#{quantity}) for #{item}."
    return false
  end
end

# 2.2.3 :039 > uploaded_quantity_valid?(quantity, item)
# Invalid quantity (9.99) for bobots, must be a positive number.
#  => false
# 2.2.3 :040 >

I like working on bugs.  I always learn something when trying to fix things.

Learn Something New Everyday

Last Edited by: John on December 09, 2015