current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

I have 2 tables as below:

Downtime

enter image description here

DowntimeCode

enter image description here

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?

share|improve this question

2 Answers 2

up vote 10 down vote accepted

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.

share|improve this answer
    
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 at 17:48
    
Cool! Feel free to mark any useful answer as accepted :) –  Mat's Mug Feb 7 at 18:10
up vote 6 down vote

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();                
    }
}
share|improve this answer
    
instead of creating the x variable you could just say where r.Code == item.Code –  Malachi Feb 7 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 at 17:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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