Figuring out delivery times

Is there anything that can be done to improve this code? Even if its using a comepltey different technique.

This code gets a list of vanruns with rules, cutoff time and fittinging time offset. We pass in the date (either now or for future that works out the best vanrun time) We want the latest cutofftime in that day for the same warehouse. Generally is this code OK for long term use or should it be broken down more? or possibly the entire logic is flawed? Use different mechanics? Just a review if anything seriously bad is happening.

```
private ServiceData.Models.CalculatedCutOff VarnRunEngine(List vanRuns, DateTime matchDate)
{
//get the date we are interested in
System.DayOfWeek dayOfWeek = matchDate.DayOfWeek;
DateTime dateOfWeekLoop = new DateTime(matchDate.Ticks);

if (vanRuns == null | (vanRuns != null && vanRuns.Count == 0))
throw new Exception("There are no Vanruns available to work out fitting times");

Models.VanRunPrototype.VanRun vr = null;

int loopkill = 0;
while (true)
{
loopkill += 1;

var _vrs = vanRuns.Where(v => v.DayOfWeek == (int)dateOfWeekLoop.DayOfWeek).ToList();
var _vrss = _vrs.Where(v => v.CutOff > dateOfWeekLoop.TimeOfDay).ToList();
var _vrsss = _vrss.OrderBy(t => t.CutOff).ToList();

if (_vrsss != null && _vrsss.Count > 0)
{

foreach (var v in _vrsss)
{
if (v.CutOff > DateTime.Now.TimeOfDay &&
(vr == null || (vr.FittingFor vr.CutOff && v.WareHouseID == vr.WareHouseID))))
{
vr = v;
}
}

vr = vr ?? _vrsss.First();
break;
}

//Go to next morning
dateOfWee

There are somethings that I want to point out, that you are doing slightly wrong.

• The first one is the argument checking. Argument checking should always be the first thing you do in your code, unless it is an expensive operation or you can't do it in a straight forward manner. In your case you don't have those reasons, so move your argument checking to the beginning. You should also use ArgumentException when your arguments are invalid instead of Exception.

• The second one is that you are not following the camelCase convetion for your variables, you are using _camelCase convention which applies to instance fields. Even if you are following your own convention which is not bad, I would not recommend a _camelCase convention.

• You shouldn't call the method ToList from LINQ so often. Calling the method ToList transforms what may be a lazy collection to a eager collection (which occupy the memory).

Here follows the review I made to your code, I post some comments regarding some decisions I made. The code I provide may be further reviewed but I think this is a fair starting point.

ServiceData.Models.CalculatedCutOff VarnRunEngine(List vanRuns, DateTime matchDate)
{
    if (vanRuns == null || vanRuns.Count == 0)
        throw new ArgumentException("There are no Vanruns available to work out fitting times", vanRuns);

    System.DayOfWeek dayOfWeek = matchDate.DayOfWeek;
    DateTime dateOfWeekLoop = new DateTime(matchDate.Ticks);
    Models.VanRunPrototype.VanRun vr = null;
    int maxIterations = 14;
    for(int i = 0; i  v.DayOfWeek == (int)dateOfWeekLoop.DayOfWeek &&
                                     v.CutOff > dateOfWeekLoop.TimeOfDay)
                         .OrderBy(t => t.CutOff);

        //vrs will never be null, it may be empty though
        //But you don't really have to check count because you will be iterating it, meaning that there is no problem
        //if vrs is empty. But if you want to preserve this check for any reason you could use vrs.Any()
        //if (vrs != null && vrs.Count() > 0)
        //{
            foreach (var v in vrs)
            {
                if ((vr == null || (vr.FittingFor  vr.CutOff && 
                                    v.WareHouseID == vr.WareHouseID)))

                {
                    vr = v;
                }
            }

            vr = vr ?? vrs.First();
        //}

        //Go to next morning
        dateOfWeekLoop = dateOfWeekLoop.Date + new TimeSpan(8, 0, 0);
        dateOfWeekLoop = dateOfWeekLoop.AddDays(1);
    }

    if(vr == null){
        throw new Exception(string.Format("Couldn't calculate CutOff in {0} iterations", maxIterations));
    }

    DateTime co = dateOfWeekLoop.Date + vr.CutOff;
    return new TyresAndServiceData.Models.CalculatedCutOff()
    {
        CutOffTime = co,
        FittingFor = co.AddMinutes(vr.FittingIn).AddMinutes(vr.FittingInAdjust ?? 0),
        QueryDate = matchDate,
        WareHouseID = vr.WareHouseID ?? 0
    };

}

StackExchange Code Review Q#41057, answer score: 4