-1
\$\begingroup\$

I have a Rails app where we generate calls. Each call has an incident_number which is a unique number based off the following format: 14-00001 (year in 2 digit followed by a dash then a 5 digit number).

The following code generates an incident number for the Call record by getting the incident year, stripping it to two digits, checking if there are any call records and if the count is 0 then generate the first number as 00001 otherwise it pulls the last incident_number and increments by 1.

call.rb model

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

This code works fine and as designed but I'd like to refactor it somehow to do the following.

When the next year rolls around i.e 2015, the first part of the number is 15 but I'd like to reset the call incident_number sequence so that the first call of the year is 15-00001 instead of 15-10233 (as an example). So far each time the new year rolls around I have to manually reset the incident number of the first call of the year to i.e. 14-00001. I'd like to find a way programmatically to check if the call is the first call of the new year and reset the incident_number to 15-00001.

\$\endgroup\$

closed as off-topic by 200_success Sep 8 '16 at 0:53

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the examples and pointing out the flaws in my code. I wish I would have thought about the possibility of a race condition two years ago when I wrote this function. I'll definitely look into adding a uniqueness constraint in the database, that will prevent the duplicate incident_number. I think refactoring and adding the scope makes sense. But there's still the issue with resetting the sequence number at the beginning of the year. Any thoughts on how I could accomplish this? \$\endgroup\$ – nulltek Sep 7 '14 at 23:21
  • \$\begingroup\$ @cz3ch That was actually in my answer, although a little hidden (and with a stupid hardcoded value). The generate_incident_number method uses the by_incident_year scope to find the currently highest incident number within a given year (I'd hardcoded 14 as the scope argument, but just changed it to Date.current.year % 100 so it'll actually follow the current year). Point is, it'll start at 1 if there are no calls with incidence numbers starting with 14- (or 15- next year etc.) \$\endgroup\$ – Flambino Sep 7 '14 at 23:43
  • \$\begingroup\$ @cz3ch welp, there was some more nonsense in the code, but it's fixed now \$\endgroup\$ – Flambino Sep 8 '14 at 1:03
1
\$\begingroup\$

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
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure this addresses my question on how to reset the incident number to 00001 at the beginning of the year. Or am I not understanding your answer? Thanks! \$\endgroup\$ – nulltek Sep 7 '14 at 23:35
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

We can simply use the built-in methods for this.

"#{Time.now.strftime('%y')}-00001".succ
  • strftime('%y') gives 2 digit year
  • succ gives the next in sequence. works for 00000 too

Sample output from irb:

irb(main):008:0> "#{Time.now.strftime('%y')}-00570".succ
=> "16-00571"

PS: though this is dated, I added the answer for anyone who stumbles upon this.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.