Finding neighbors and returning average value from neighbors

Question

I have a list of sales records sorted by sales date. I need to fill in the gaps (days without a sales record) in this list with 'inferred sales' records. The method in question calculates an average price for the inferred sales records from its closest valid neighbors that have the same SalesType. If there's only one neighbor then no need to average it just returns the sales price of the neighbor. I'd like to improve readability of this method.

public class SalesRecord
{
    public DateTime Date { get; set; }
    public bool IsValid { get; set; }
    public int SalesType { get; set; }
    public double UnitPrice { get; set; }
}
public class SalesDataContainter
{
    private SortedList<DateTime, SalesRecord> salesData;
    private double GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        SalesRecord closestBefore = null;
        SalesRecord closestAfter = null;
        foreach (var salesRecord in salesData.Values)
        {
            //neigbor needs to be valid
            if (salesRecord.IsValid)
            {
                continue;
            }
            //has to have the same sales type
            if (salesRecord.SalesType != inferredSalesRecord.SalesType)
            {
                continue;
            }
            //listed is sorted get the last found for before neighbor
            if (salesRecord.Date < inferredSalesRecord.Date)
            {
                closestBefore = salesRecord;
            }
            //listed is sorted get the first found for after neighbor
            if (closestAfter == null && salesRecord.Date > inferredSalesRecord.Date)
            {
                closestAfter = salesRecord;
            }
        }
        // we have neighbors on both sides
        if (closestBefore != null && closestAfter != null)
        {
            return (closestAfter.UnitPrice + closestBefore.UnitPrice) / 2;
        }
        // there's only before neighbor
        if (closestBefore != null)
        {
            return closestBefore.UnitPrice;
        }
        // there's only after neighbor
        if (closestAfter != null)
        {
            return closestAfter.UnitPrice;
        }
        return 0;
    }
}

Answer 1

I would do something like this:

public class SalesRecord
{
    public DateTime Date { get; set; }
    public bool IsValid { get; set; }
    public int SalesType { get; set; }
    public decimal UnitPrice { get; set; }
}
public class SalesDataContainter
{
    private SortedList<DateTime, SalesRecord> salesData;
    private decimal GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        // select valid records of the same type as inferredSalesRecord
        var candidateRecords = salesData.Values.Where(r => r.IsValid && r.SalesType == inferredSalesRecord.SalesType);

        SalesRecord closestBefore = candidateRecords.LastOrDefault(r => r.Date < inferredSalesRecord.Date);
        SalesRecord closestAfter = candidateRecords.FirstOrDefault(r => r.Date > inferredSalesRecord.Date);

        // we have neighbors on both sides
        if (closestBefore != null && closestAfter != null)
        {
            return (closestAfter.UnitPrice + closestBefore.UnitPrice) / 2m;
        }
        // there's only before neighbor
        if (closestBefore != null)
        {
            return closestBefore.UnitPrice;
        }
        // there's only after neighbor
        if (closestAfter != null)
        {
            return closestAfter.UnitPrice;
        }
        return 0m;
    }
}

Answer 2

Just a few things I want to point out before tackling this:

• Based on the comment, your first condition is flipped. You're skipping valid entries instead of invalid entries. It should be:

  if (!salesRecord.IsValid)
  

• Your last condition in the loop could just break at that point. Since it is sorted, every valid item will be a match but you're only interested in the first.

• You should consider making UnitPrice of type decimal. You're dealing with currency here so you should not be using double.

Using a little bit of LINQ here could make this more readable IMHO. It's easier to think of it as "looping through the list of candidate records" rather than "looping through all records and skipping some of them."

Personally, I would write the last set of conditions as I have shown using the conditional operator. You may or may not like it this way or this structure so your choice with that part.

public class SalesRecord
{
    public DateTime Date { get; set; }
    public bool IsValid { get; set; }
    public int SalesType { get; set; }
    public decimal UnitPrice { get; set; }
}
public class SalesDataContainter
{
    private SortedList<DateTime, SalesRecord> salesData;
    private decimal GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        Func<SalesRecord, bool> isValidRecord = record =>
            record.IsValid                                          //neigbor needs to be valid
            && record.SalesType == inferredSalesRecord.SalesType;   //has to have the same sales type
        var candidateRecords = salesData.Values
            .SkipWhile(record => !isValidRecord(record))
            .Where(isValidRecord);

        SalesRecord closestBefore = null;
        SalesRecord closestAfter = null;
        foreach (var salesRecord in candidateRecords)
        {
            //get the last found for before neighbor
            if (salesRecord.Date < inferredSalesRecord.Date)
            {
                closestBefore = salesRecord;
            }
            //get the first found for after neighbor
            else if (salesRecord.Date > inferredSalesRecord.Date)
            {
                closestAfter = salesRecord;
                break;
            }
        }

        // there's a neighbor before
        if (closestBefore != null)
        {
            return (closestAfter != null)
                // we have neighbors on both sides
              ? (closestBefore.UnitPrice + closestAfter.UnitPrice) / 2M
              : closestBefore.UnitPrice;
        }
        else
        {
            return (closestAfter != null)
                // there's only a neighbor after
              ? closestAfter.UnitPrice
              : 0M;
        }
    }
}

An alternative would be to let LINQ do all the work:

public decimal GetInferredPrice(SalesRecord inferredSalesRecord)
{
    Func<SalesRecord, bool> isValidRecord = record =>
        record.IsValid                                          //neigbor needs to be valid
        && record.SalesType == inferredSalesRecord.SalesType;   //has to have the same sales type

    return salesData.Values
        .SkipWhile(record => !isValidRecord(record))
        .Where(isValidRecord)
        .GroupBy(
            record => record.Date.CompareTo(inferredSalesRecord.Date),
            record => record.UnitPrice as decimal?,
            (key, g) => (key < 0) // assumes no records will have equal dates
                ? g.LastOrDefault()
                : g.FirstOrDefault()
        )
        .Average() ?? 0M;
}

Answer 3

I'm a fail-fast fan, so I'd code GetInferredPrice as such:

    private double GetInferredPrice(SalesRecord inferredSalesRecord)
    {
        var candidateRecords = this.salesData.Values.Where(salesRecord => salesRecord.IsValid
            && (salesRecord.SalesType == inferredSalesRecord.SalesType));
        var closestBefore = candidateRecords.LastOrDefault(salesRecord => salesRecord.Date < inferredSalesRecord.Date);
        var closestAfter = candidateRecords.FirstOrDefault(salesRecord => salesRecord.Date > inferredSalesRecord.Date);

        // we have no neighbors on either side
        if ((closestBefore == null) && (closestAfter == null))
        {
            return 0;
        }

        // only before neighbor, only after neighbor, or both
        return closestAfter == null
            ? closestBefore.UnitPrice
            : (closestBefore == null
                ? closestAfter.UnitPrice
                : (closestAfter.UnitPrice + closestBefore.UnitPrice) / 2);
    }

I don't know the full usage of SalesRecord, but despite the lack of good immutable semantics, I still like immutable classes if you can swing it:

public sealed class SalesRecord
{
    private readonly DateTime date;

    private readonly bool isValid;

    private readonly int salesType;

    private readonly double unitPrice;

    public SalesRecord(DateTime date, bool isValid, int salesType, double unitPrice)
    {
        this.date = date;
        this.isValid = isValid;
        this.salesType = salesType;
        this.unitPrice = unitPrice;
    }

    public DateTime Date
    {
        get
        {
            return this.date;
        }
    }

    public bool IsValid
    {
        get
        {
            return this.isValid;
        }
    }

    public int SalesType
    {
        get
        {
            return this.salesType;
        }
    }

    public double UnitPrice
    {
        get
        {
            return this.unitPrice;
        }
    }
}