###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).