Return to Answer

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

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

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
}

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.

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

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

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
}

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.

Besides this, I think that what you are doing here is reasonable. However, I would also like to point out that if you are using a "real" RDMS here, such as MS SQL Server, MySQL, PostgreSQL or whatever, 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. If only ShiftId is different, that will be fine, and if only ShiftHour is different that will be fine, but if both of them are the same then your RDMS will not under any circumstances allow you to insert a new row into the table, as it violates this constraint.

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
}

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.

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