-1

I'm working on some code that checks for some invariants in the database. For example to check that an element is not repeated the code checks for a database error since the invariant is coded at database level.

Basically to enforce uniqueness for a certain set of key the code checks for the error:

if (sqlError = "name_age") { // name+age are non unique cannot insert
  throw error("no users with same name and age allowed")
}

To me it was a bit unreadable, mostly because it must be read together with the database configuration, furthermore, it cannot be tested without a mock or by hitting the db itself. What I would like to do is:

if (countWithNameAndAge(name, age) == 1) {
  throw error("no users with same name and age allowed")
}

Not only this is more immediately readable but I can allow 2 or 3 people with same name and age without evolving the database or adding columns.

The only cons is that this may be slower.

Is it true that my approach may be slower? Is it indeed more legible?

  • 1
    1) I'd avoid relying on exact text messages where possible; think maintenance. 2) An INSERT throwing an error is atomic, a separate query is not. If there is a chance for more than one writer, some kind of locking must take place. 3) If the database is of nontrivial size, index operations and inserts are certainly really the key factor, so acquiring a lock, performing a query, inserting a record and releasing the lock may be comparable, since the UNIQUE thingy has to do essentially the same. But I'd conduct a benchmark. 4) Disadvantage: Implementation of the requirement is distributed. – unified modeling sandwich Jun 7 '17 at 14:05
  • instead of the literal error message string you could check against a constant: if (sqlError.equals( NAME_AND_AGE_ALREADY_IN_DB)){ – Timothy Truckle Jun 7 '17 at 14:08
  • The error message was explicative. I updated the question – gurghet Jun 7 '17 at 14:58
  • @ClassStacker, your comment should be an answer. – Erik Eidt Jun 7 '17 at 17:06
  • 1
    I wouldn't use age anywhere, since it is relative to now (use birthYear instead). I also don't think name and age/year are good uniqueness criteria; here's hoping this explicative and orthogonal to the actual question of how/when to detect/ensure (uniqueness) constraints. – Erik Eidt Jun 7 '17 at 17:12
1

Performance vs. Readability

Most likely, the single INSERT will not be significantly faster than a cleverly coded LOCK - FETCH - INSERT - UNLOCK sequence, because the UNIQUE index enforces the same operations, and the embedded INSERT should be faster than the standalone INSERT. But if performance is important to you, you should conduct a benchmark. Searching the index for existing records and inserting the data in the presence of the index (having to update the index) are the expensive operations here.

Maintainability

Comparing the SQL error variable against a string is a mainenance horror. It would be better if the INSERT statement return code would already give an indication what went wrong, such that it can be evaluated. This would be my preferred approach (keeping it simple).

If that is not possible, comparing the error text to a string constant might be a compromise, as suggested by @TimothyTruckle.

Sticking with a single, potentially failing INSERT also has the advantage of not distributing the implementation of the uniqueness requirement to several levels. Strictly speaking, with the single INSERT, this essential invariant is guaranteed by the UNIQUE index, and the separate error message is just being nice to the user.

Data Model

As @ErikEidt and @RSahu suggest, age is not the best choice here. It should not be stored, and it is not suitable for checking duplicates most of the time.

  • @ErikEidt Thank you. I have treid to summarise everything. – unified modeling sandwich Jun 7 '17 at 17:45
  • I voted this answer because of the bold text – gurghet Jun 9 '17 at 8:22
1
if (countWithNameAndAge(name, age) == 1)

has to go through each item in the data base to come up with a count. You could change that to:

if (hasNameAndAge(name, age))

This will return at the first occurrence of an item with the given name and age, and hence, will be a little bit faster. However, you lose the flexibility of being able to allow more than 3 people with the same name and age. If that flexibility is a must, you can tweak hasNameAndAge to add a count also.

if (hasNameAndAge(name, age, MAX_COUNT))

That will return with true as soon as it finds MAX_COUNT number of items.

Having said that, I wonder. Should you even worry about the performance ramifications of detecting duplicates? What is the percentage of times that you expect you/your users to be attempting to add non-duplicates vs duplicates? If the percentage of times duplicates are attempted to be added is small, the performance issue of detecting duplicates is moot.

It seems to be that you need to optimize the hasNameAndAge, no matter which version you choose, when you expect it to return false if percentage of times duplicates are attempted to be added is small.

  • I wouldn't expect countWithNameAndAge(name, age) to be any slower because there's a UNIQUE (name, age) index and besides, in the presence of the index, it doesn't have to "go through each item in the database". – unified modeling sandwich Jun 7 '17 at 17:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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