Refactor auto-incrementing function in Ruby on Rails

After doing some research and trying out different methods I came up with this to address the incident_number count and the reset of of the incident_number sequence each year. Note, this does not address the possibility of a race condition but I think it's making some headway

  def generate_incident_number
    incident_year = Time.now.year.to_s[2..-1]
   if Call.count == 0
    self.incident_number = "#{incident_year}-00001"
   else
    last = Call.last.incident_number
   end
   if(!last || last.split('-')[0] != incident_year)
    self.incident_number = "#{incident_year}-00001"
   else
    number = last.split('-')[1].to_i
    number += 1
    self.incident_number = incident_year + '-' + "%05d" % number
  end
end

I would love some feedback on this refactor. Also any thoughts on how it might affect two years worth of Call records in the database. i.e. possible skewing of the sequence or a gap in numbers. I think a good option would to be copying over the production database from Postgres into my dev environment, and giving it a test.

You can only avoid a race condition by doing the increment in the database. If you want to avoid a RecordNotUnique error, and want to avoid using proprietary database extensions that are incompatible with a schema.rb dump, you need to put the increment in a transaction. The following code will do so, although you'll need to modify it to prepend the year.

before_create :increment_identifier

  private

  def increment_identifier
    sql = 'SELECT COALESCE(MAX(identifier), 0) + 1 FROM claims'
    self.identifier = ActiveRecord::Base.connection.execute(sql).values[0][0]
  end

The incident number is really a view concern or a domain model concern, not a database model one.

That is, you should probably use an ordinary auto-inc primary key and a timestamp in your database, and have a method that calculates your incident number based on those. This immdediately solves the race condition problems Flambino mentioned, and makes everything simpler.

You incident number becomes last 2 of year plus the difference between the record's pk and the pk of the first record of the year, plus one, formatted to five digits.

If you need the incident numbers to be exactly seqential with no possiblity of gaps (from deleted records, eg), then you can do a count of all records from the year with a pk smaller than the record in question to get that record's incident number for the year.

There's a potential race condition: Two requests both get the last incident number at the same time, thus both producing the same incremented number, and you end up with two calls with the same number.

As for the year-rollover, the trouble, I think, is that you're treating this as a single value when it's actually two.

So instead, have two attributes/columns: One for the year, one for the sequence number. Only generate the incident_number when requested. E.g. if we reuse the conventional created_at (though it might be better with a separate column):

def incident_number
  number = "00000#{sequence_number}"
  "#{created_at.year % 100}-#{number[-5,5]}"
end

And in case you need to find a record by its incident_number you might do something like this (assuming MySQL)

scope :by_incident_number, -> (string) {
  year, sequence = string.split(/-0+/)
  where("YEAR(created_at) = ? AND sequence_number = ?", "20#{year}", sequence)
}

Of course, a simpler way that doesn't involve a large database migration is to use a scope to begin with:

scope :by_incident_year, -> (year) { # two-digit year
  where("incident_number LIKE ?", "#{year}-%")
}

If you pass it 14 it'll match rows with incident numbers like 14-*. To get the next incident number you could do

def generate_incident_number # don't call it genrate_..._id when the column's called ..._number
  # the current 2-digit year
  year = Date.current.year % 100

  # highest number for the current year (may be nil)
  max = Call.by_incident_year(year).maximum(:incident_number)
  sequence_number = max ? max.scan(/\d+$/).first.try(:to_i) : 0

  # set the incremented number
  self.incident_number = "#{year}-#{(sequence_number || 0) + 1}"
end

But (safely) generating a new sequence number, that's trickier. MySQL has auto-increment of course, but that's table-wide, and won't reset year over year.

First of all, I'd add a uniqueness constraint in the database. Something like this migration

# if you use one column
add_index :calls, [:incident_number], unique: true

# or if you use two columns
add_index :calls, [:year, :sequence_number], unique: true

Beware: Check if you already have conflicts before trying to add constraints.

From here, you can either do the incrementing in the database using some SQL, or in Ruby as shown above. But in both cases, should the constraint be tripped, you'll get exception in Rails.

Basically, if you try to create/save a record with a duplicate incident_number (or duplicate year and sequence_number columns), you'll get a ActiveRecord::RecordNotUnique exception (if I recall correctly), which you can handle appropriately (e.g. re-generate a new number, or simply bounce back to whatever form it came from and let the user re-submit; it'll happen so rarely that that's probably ok to do).

It's not pretty, but it should be safe.

You have a line where you parse out last incident number:

number = last_incident_number.split('-')[1].to_i

Add similar code to parse last incident year:

last_incident_year = last_incident_number.split('-')[0].to_i

And then compare it to the incident_year variable.