3
\$\begingroup\$

I have these classes:

public class DelayReason
{
    public virtual ICollection<WorkOrder> WorkOrders { get; set; }

}
public class WorkOrder 
{
    public virtual ICollection<DelayReason> DelayReasons { get; set; }
 //other fields
}

Each DelayReason has 0..* WorkOrders and it's the same for WorkOrders.

This is a data sample:

enter image description here

To do this, I've written this code. Can I optimize that?

  public void DelayReasonAverage()
    {
        List<ReportVM> result = new List<ReportVM>();

        var reasons = _delayReasons.GroupBy(row => row, (k, t) => new
        {
            delayId = k.Id,
            delayTitle = k.Title,
            workOrders = k.WorkOrders,
            count = t.Count()
        }).OrderByDescending(row => row.count).ToList();

        reasons.ForEach(row =>
        {
            var workOrders = row.workOrders.ToList();

            ReportVM reportObj = new ReportVM() { DelayTitle= row.delayTitle };

            for (int i = 0; i < workOrders.Count; i++)
            {
                if (workOrders.Count - 1 > i)
                    reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
            }
            reportObj.Count = reportObj.Count / row.count;
            result.Add(reportObj);
        });
    }

DiffDays extension Method:

 public static int DiffDays(this DateTime current,DateTime to)
 {
    return to.Subtract(current).Days;
 }

The final results look like this:

enter image description here

\$\endgroup\$
7
  • \$\begingroup\$ below sample is my goal you mean this is the result, right? If this is your goal then the question is off-topic because your code is not doing this yet. \$\endgroup\$
    – t3chb0t
    Commented Apr 29, 2017 at 10:35
  • \$\begingroup\$ @t3chb0t is it wrong ? where ? \$\endgroup\$ Commented Apr 29, 2017 at 10:37
  • \$\begingroup\$ @t3chb0t Updated , please see new update \$\endgroup\$ Commented Apr 29, 2017 at 10:56
  • 2
    \$\begingroup\$ Now it looks good ;-) \$\endgroup\$
    – t3chb0t
    Commented Apr 29, 2017 at 10:56
  • 1
    \$\begingroup\$ I think you might have a bug in your for loop. Since i < workOrders.Count then this if (workOrders.Count - 1 > i) is never true and the reportObj.Count is never increased. \$\endgroup\$
    – t3chb0t
    Commented Apr 29, 2017 at 11:42

3 Answers 3

2
\$\begingroup\$

While the logic of this:

for (int i = 0; i < workOrders.Count; i++)
{
    if (workOrders.Count - 1 > i)
        reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
}
reportObj.Count = reportObj.Count / row.count;

is valid. I think the for loop might not be necessary. You should be able to take the difference from the first to last:

reportObj.Count = workOrders[0].CreatedOn.DiffDays(workOrders[workOrders.Count - 1].CreatedOn) / row.count;

In looking at the rest of that method it looks like the LINQ queries can be simplified quite a bit. Something like this should work:

public List<ReportVM> DelayReasonAverage()
{
    return (from DelayReason dr in _delayReasons
            let row = new
            {
                delayId = dr.Id,
                delayTitle = dr.Title,
                workOrders = dr.WorkOrders.ToList(),
                count = _delayReasons.Count()
            }
            orderby row.count descending
            let reportObj = new ReportVM()
            {
                DelayTitle = row.delayTitle,
                Count = (int)Math.Round(row.workOrders[0].CreatedOn.DiffDays
                (row.workOrders[row.workOrders.Count - 1].CreatedOn) / (double)row.count)
            }
            select reportObj).ToList();
}

I noticed your post has this method as void, but it's creating a list, so I changed the signature to reflect that. I also changed the way the average is calculated, by using Math.Round to get a more accurate reflection of the actual average.

\$\endgroup\$
1
\$\begingroup\$

Do you need the GroupBy? I don't see a reason for that. So I would do it that way:

public IList<ReportVM> DelayReasonAverage(IList<DelayReason> delayReasons)
{
    // Create the List with the appropriate capacity
    IList<ReportVM> reportVMs = new List<ReportVM>(delayReasons.Count);

    foreach (DelayReason delayReason in delayReasons.OrderByDescending(dr => dr.WorkOrders.Count))
    {
        int delaySumInDays = 0;

        // This .ToList() could be prevented when delayReason.WorkOrders could be IList<WorkOrder>
        var wo = delayReason.WorkOrders.ToList();
        for (int i = 0; i < wo.Count - 1; ++i)
        {
            // Why need the DiffDays-Method? For me this is very readable
            delaySumInDays += (wo[i + 1].CreatedOn - wo[i].CreatedOn).Days;
        }

        reportVMs.Add(new ReportVM
        {
            Title = delayReason.Title,
            AverageDays = delaySumInDays / wo.Count
        });
    }

    return reportVMs;
}
\$\endgroup\$
0
\$\begingroup\$

This

        for (int i = 0; i < workOrders.Count; i++)
        {
            if (workOrders.Count - 1 > i)
                reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
        }

Could be

        for (int i = 0; i < workOrders.Count - 1; i++)
        {
            reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
        }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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