# Optimising and error handling Linq query

I have 2 tables as below:

Downtime

DowntimeCode

I am then doing this to get the Description field from the DowntimeCode table for the DowntimeCode with the sum highest value in the Downtime table for the current date and a specified line:

public string TopCodeForToday(string line)
{
var date = DateTime.Now;

using (var db = new PillowContext())
{
var qry = (from d in db.DownTimes
where DbFunctions.TruncateTime(d.DateTime) == DbFunctions.TruncateTime(date) && d.Line == line
group d by new { d.Code }
into g
let total = g.Sum(x=>x.Amount)
orderby total descending
select new { g.Key.Code, Total = total }).ToList();

for (int i = 0; i <= 1; i++)
{
foreach (var item in qry)
{
int x = item.Code;
var result = from r in db.DownTimeCodes
where r.Code == x
select r.Description;

return result.FirstOrDefault();
}
}
}
return "Fail";
}


Now this is working OK but I can't help but think there is a simpler way of doing it. I'm also not too hot on error handling (hence there not being any). Can anyone make any recommendations on how to improve and streamline the above?

-

Not having any error handling in your TopCodeForToday() method, means if anything goes wrong, you're putting the burden of catching any exceptions onto the caller.

And this isn't necessarily a bad thing. It depends on what the client code is expecting.

We don't know what class this method is a member of. The problem is that the naming of the method doesn't convey any information about what's going on, and whether the calling code should wrap it with a try/catch block. If the class is clearly an object that accesses a database and only does SELECT operations, then it's probably fine. Otherwise the method's name should say what it's doing - SelectTopCodeForToday() would be a better name regardless of all of the above, be it only because it starts with a verb.

I don't think returning a string that says "Fail" is a good idea. Replace FirstOrDefault() with First() and let it throw an exception, and then your client code will know that something is wrong with the data.

The problem with returning a "Fail" string, is that it's a valid string. And then the client code needs to verify whether the function returned "Fail" or not. By throwing, you only handle the "happy" path, making your code more focused; and you handle exceptional situations with ...exceptions.

-
Thank you for the explanation. Makes sense. This class is just performing select statements but now you come to mention it the naming convention isn't ideal. The "Fail" string was just a placeholder I put there for now until I could work out how to error handle the method properly. – Jimsan Feb 7 '14 at 17:48
Cool! Feel free to mark any useful answer as accepted :) – Mat's Mug Feb 7 '14 at 18:10

I don't understand your for and foreach loops - you always return from the first iteration!

What's wrong with this code?:

public string TopCodeForToday(string line)
{
var date = DateTime.Now;

using (var db = new PillowContext())
{
var qry = from d in db.DownTimes
where DbFunctions.TruncateTime(d.DateTime) == DbFunctions.TruncateTime(date) && d.Line == line
group d by new { d.Code }
into g
let total = g.Sum(x=>x.Amount)
orderby total descending
select new { g.Key.Code, Total = total };

var item = qry.FirstOrDefault();
if (item == null) {
return "Fail";
}
int x = item.Code;
var result = from r in db.DownTimeCodes
where r.Code == x
select r.Description;

return result.FirstOrDefault();
}
}

-
instead of creating the x variable you could just say where r.Code == item.Code – Malachi Feb 7 '14 at 17:10
Sorry, totally agree with you. I had a brain fart and forgot about FirstOrDefault so originally I was trying to do it with result.ToString (hence the need for the loops to narrow it down to 1 result). I then realised that this will return the actual query string so amended it but forgot to take the loops out. – Jimsan Feb 7 '14 at 17:41