'Better' way to handle adding a record

Scenario: I am using a helper class called 'OutputHelper' to add a new record to the table 'Output'. My 'Output' object contains the following:

• ShiftID

• Model

• WorksOrder

• LotNo

• Quantity

• ShiftHour

I do not want a record to be added if a record already exists with the ShiftID and ShiftHour of the record that I am trying to add.

The way I am achieving this currently is having the property 'RecordOutput' in the helper class try and add the record then return a string which I handle with a switch statement and provide feedback to the user accordingly.

This seems to be working fine but I can't help but think it's a terrible way of doing things. Can anyone provide any feedback and suggestions for improvement?

Output helper class:

public class OutputHelper
{
    public string RecordOutput(Models.Output output)
    {
        var shiftId = output.ShiftID;
        var shiftHour = output.ShiftHour;

        using (var db = new TestContext())
        {
            try
            {
                if (db.Outputs.Any(x => x.ShiftID == shiftId && x.ShiftHour == shiftHour))
                {
                    return "Exists";
                }
                db.Outputs.Add(output);
                db.SaveChanges();
            }
            catch (Exception)
            {
                return "NotSaved";
            }
            return "Success";
        }
    }
}

Code from button click on form

```
var outputHelper = new OutputHelper();

switch (outputHelper.RecordOutput(output))
{
case "Exists":
lblErrorMessage.Text = "Output already recorded for this timeframe";
failMessage.Visible = true;
break;
case "Not Saved":
lblErrorMessage.Text = "There was a problem saving this record. Please try again";
failMessage.Visible = true;
break;
default:
lblSuccess.Text = "Output record added to

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

StackExchange Code Review Q#41572, answer score: 8