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:
- 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.
- The query produces one result: The result is used.
- 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.
context.update({Object} commuteData);
\$\endgroup\$ – Vogel612♦ Mar 31 '14 at 14:36