Class to Return Single Integer

Question

I have a class used to return a single integer, but think there is room for improvement. The class should only return a single row every time and if no row is found it might be a good idea to throw an error from the class rather than from the calling code which will break when the integer comes back as zero.

The function below is called from a sync application that is returning a primary key in order to make an update.

    public static int GetCommuteDataID(CommuteData commuteData)
    {
        int commuteDataID = 0;
        using (var db = new CarbonContext())
        {
            var returnCommuteData = from e in db.CommuteDatas
                              where e.id == commuteData.id &&
                                    e.SurveyID == commuteData.SurveyID &&
                                    e.SurveyYear == commuteData.SurveyYear
                              select e.CommuteDataID;

            foreach (var item in returnCommuteData)
                commuteDataID = Convert.ToInt32(item.ToString());
        }
        return commuteDataID;
    }

Here is the code that calls GetCommuteDataID:

//Make sure the record does not exist
if (!CommuteDataRepository.CommuteDataExists(localCommuteData))
    db.CommuteDatas.Add(localCommuteData);
else
{
    localCommuteData.CommuteDataID = CommuteDataRepository.GetCommuteDataID(localCommuteData);
    db.Entry(localCommuteData).State = System.Data.EntityState.Modified;
}

Here is the new function:

public static int GetCommuteDataID(int? id, int surveyID, int surveyYear)
{
    using (var db = new CarbonContext())
    {
        return db.CommuteDatas
                 .Where(cd => cd.id == id &&
                              cd.SurveyID == surveyID &&
                              cd.SurveyYear == surveyYear)
                 .Select(cd => (int)cd.CommuteDataID)
                 .SingleOrDefault();
    }
}

Answer 1

I see a number of things wrong with this implementation:

First, You already have a CommuteData entity. Why are you using it to go back to the database to retrieve a property that's already on it (ie: commuteData.CommuteDataId)? This makes me very suspicious of any calling code (you may want to post a use case for clarity).

That point aside, if there's a possibility that the method will be unable to return, it should throw an exception if an invalid state is encountered, or not return a number that is invalid (ie: 0).

To combat this, you could go with one of these patterns:

// Throw an exception if there is no applicable CommuteDataId but one
// is always expected to be available when calling this method:
public static int GetCommuteDataId(CommuteData commuteData)
{
    ...
    // Tell EF that you only expect one and only one result.
    return something.Single()
}

// Return null if there is no applicable CommuteDataId...
public static int? GetCommuteDataId(CommuteData commuteData)

// Return true if commuteDataId contains a valid Id.
public static bool TryGetCommuteDataId(CommuteData commuteData, out commuteDataId)

The property names you're using are also very confusing. What's the difference between CommuteData.id and CommuteData.CommuteDataID? Furthermore, the casing of your properties are not consistent: id, CommuteDataID, SurveyId.

This code is also very suspicious:

commuteDataID = Convert.ToInt32(item.ToString());

It would appear that you are converting an int to a string, then back to an int. If it's not an int, the method should return it in it's true form instead of doing any misleading conversion that the caller has no idea about.

Lastly, you're pulling back too much information from the database. SQL (and thus EF) can return exactly what you want in a single call.

public static int? GetCommuteDataID(int commuteDataId)
{
    using (var db = new CarbonContext())
    {
        return db.CommuteDatas
                 .Where(cd => cd.id == commuteDataId)
                 .Select(cd => (int?)cd.CommuteDataID)
                 .SingleOrDefault();
    }
}

The updated example above still has several problems with it, but hopefully it illustrates some of the points I was trying to make.

UPDATE:

Your updated function is almost there, but there are still some things wrong/misleading about it:

public static int GetCommuteDataID(int? id, int surveyID, int surveyYear)

Why is id nullable here? This implies that either:

• If id isn't provided, surveyId and surveyYear are enough to produce a unique result (otherwise you wouldn't be returning a single int).
• id can be used along with surveyId and surveyYear to produce a unique result even when it's null (unlikely).

Next:

.Where(cd => cd.id == id &&
    cd.SurveyID == surveyID &&
    cd.SurveyYear == surveyYear)

Again, are you sure this is the criteria you want to use to ensure uniqueness? Why would id exist if it wasn't unique?

.Select(cd => (int)cd.CommuteDataID)
.SingleOrDefault();

Lastly, if CommuteDataID is already an int there's no need to cast it to one. SingleOrDefault has three outcomes:

1. The query produces no result: The default value for the type of query is returned. When you Select an int, the default value is 0 because that's the default value for an int. I was casting to int? in my example so it would return null (the default for int?) when no value was found.
2. The query produces one result: The result is used.
3. The query produces multiple results: An exception is thrown because it was expected that only 0 or 1 results would be returned based on the filters you provided.

If your inputs should produce one and only one result, then you should use Single(), which will throw an exception if 0 or multiple results are returned by the query. All of this would imply that your method arguments were enough to produce a single result which doesn't seem to be the case.