'Better' way to handle adding a record

Question

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 database";
                successMessage.Visible = true;
                ResetForm();
                break;
        }    

Answer 1

Instead of a string, return a boolean. This simplifies the testing on the outside. For the exception case, add the information you want and then rethrow and catch in the outer method. Also, if OutputHelper is only a wrapper for the database functions, you can make the class static, which would save you from having to create an instance of it in the click method

public static class OutputHelper
{
    public static bool 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 false;
                }
                db.Outputs.Add(output);
                db.SaveChanges();
                return true;
            }
            catch (Exception e)
            {
                throw new Exception("There was a problem saving this record: " + e.Message);
            }
        }
    }
}

Then the button click code becomes:

try
{
    if (OutputHelper.RecordOutput(output))
    {
        lblSuccess.Text = "Output record added to database.";
        successMessage.Visible = true;
        ResetForm();
    }
    else {
        //to keep from duplicating the code to display the error message
        throw new Exception("Output already recorded for this timeframe.");
    }
}
catch (Exception e)
{
    lblErrorMessage.Text = e.Message;
    failMessage.Visible = true;
}

Answer 2

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

Answer 3

To piggy-back on @Simon André Forsberg answer...

Ditto, with a caveat:

Personally, I think it is perfectly OK to do a select-before-insert though.

because as soon as you read records into memory the data is stale - some other query could have added or deleted a row w/ that key an instant after you looked. So to read from the DB then test for existence in memory then attempt a DB insert is problematic.

SO... do the checking in the database. Just send the record without checking first but do something like this in the database:

IF NOT EXISTS (select * from outputTable where shiftid = passedInShiftid)
    INSERT ....
ELSE
    UPDATE ....
END IF

And as noted by Simon, the above assumes shiftid is a primary key or at least unique. Of course you can use any combination of value(s) that defines a unique record in your table.

And of course this also means that when you read a record, read the key as well, whatever it is so the above sql can be done when sending records back.

Finally whatever field(s) you use for a key, DO NOT let those values be modified in the user interface once they are created.