1
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I don't quite understand your question. Are you looking to add a new implementation to the code ? What it this code suppose to do, retrieve an id or something ? And where is the exception you're talking about ? Could clarify your question. \$\endgroup\$ – Marc-Andre Mar 31 '14 at 14:31
  • \$\begingroup\$ I don't think you need to get the primary key of an object for any update transaction. If you did things right, you should be able to just call: context.update({Object} commuteData); \$\endgroup\$ – Vogel612 Mar 31 '14 at 14:36
  • \$\begingroup\$ The function is for a sync application. I do not get my primary key value from the web service JSON that is returned. So, I see if a row exists based on three other values, then return my primary key to use for the update. \$\endgroup\$ – ADH Mar 31 '14 at 14:42
  • \$\begingroup\$ @AllanHorwitz why not change the JSON then? And doesn't this also mean you are maintaining two data-layers and storages in one application? \$\endgroup\$ – Vogel612 Mar 31 '14 at 14:59
  • \$\begingroup\$ The JSON is coming from a vendor, so I cannot change it. \$\endgroup\$ – ADH Mar 31 '14 at 15:00
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The CommuteDataID will be zero at the time the function is called, I am using the class to find that value. The "id" value is being returned from a web service. That "id" is the primary key used by the vendor of the service. CommuteDataID is my primary key. \$\endgroup\$ – ADH Mar 31 '14 at 14:39
  • 1
    \$\begingroup\$ I believe using an "invalid" entity as a query argument makes the code unclear. If the id is what makes it unique, then why also query by SurveyId and SurveyYear? If those are filter inputs, then I would add them as different function arguments instead of passing the entity itself (or only pass the id like my last example shows if it is truly a primary key). \$\endgroup\$ – Ocelot20 Mar 31 '14 at 14:54
  • \$\begingroup\$ Thank you for your help. I did originally use three arguments, but I changed it since all the values I need are already in a single argument (the object itself). The id might not be unique, all three arguments are needed. \$\endgroup\$ – ADH Mar 31 '14 at 14:59
  • 1
    \$\begingroup\$ No problem. I've updated my answer in response to your updated question that now includes more code for review. \$\endgroup\$ – Ocelot20 Mar 31 '14 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.