Skip to main content
3 of 4
added 145 characters in body
Simon Forsberg
  • 59.4k
  • 9
  • 156
  • 309

###String literals vs. constants vs. enums

I think that the biggest terrible thing that you are doing here is 1) using magic string literals that are not defined as constants. 2) using a string when you can use an enum instead.

public enum SaveResult
{
    Exists, NotSaved, Success
}

In fact, your current code returns "NotSaved", while you check for case "Not Saved":. That's why you should use constants or an enum! Also, having success as the "default" case is not something I would recommend. It'd be better to assume failure unless success has been explicitly confirmed. Or even better, default is for "Oops, I forgot to check this case explicitly!", which would help you notice that you had written "Not Saved" instead of "NotSaved".

In the end, an enum is the choice I would recommend here.

###Catching

That said and done, I really question the catch (Exception). In my experience, you should only catch the exceptions that you really need to catch. If the code would throw a NullReferenceException for example, that should propagate upwards and tell you that there is a serious bug in the code that needs to be fixed. Some Exceptions are just not meant to be caught.

###Your question, and the database aspect

Besides this, I think that what you are doing here is reasonable. I think that it is However, I would also like to point out that if you are using a RDMS here, then you might want to make your table have a primary key - or another unique index - over two columns: Both ShiftId and ShiftHour. Technically, that's the only way to really make sure that your table will not have duplicates over these values. Personally, I think it is perfectly OK to do a select-before-insert though. I think that is better than inserting without knowing if things go wrong or not, and catching the appropriate exception if things actually have gone wrong - i.e. a duplicate existed.

Be aware though, that after your SELECT query checking for a duplicate and before your INSERT query, there can theoretically be another query that just inserted a duplicate, effectively causing a race-condition. If such a situation would occur, the catch would still catch it of course (assuming you have the appropriate indexes in your table set up).

Simon Forsberg
  • 59.4k
  • 9
  • 156
  • 309