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.

I have a customer class:

public class Customer
{
    public Guid Id { get; set; }
    // Some other properties...}

And a three transactions classes that have a reference to customer, like:

public class Order
{
    public Guid CustomerId { get; set; }
    // Some other properties...}

public class Invoice
{
    public Guid CustomerId { get; set; }
    // Some other properties...}

public class Payment
{
    public Guid CustomerId { get; set; }
    // Some other properties...}

I have a collection for each of transaction type. And I want to get this collection to be "grouped" by their elements "CustomerId" properties. So, as a result I'd like to get a collection of such objects:

public class CustomerInfo
{
    public CustomerInfo(Guid customerId, IEnumerable<Order> orders,
        IEnumerable<Invoice> invoices, IEnumerable<Payment> payments){...}

    public Guid CustomerId { get; set; }
    public IEnumerable<Invoice> Invoices { get; set; }
    public IEnumerable<Order> Orders { get; set; }
    public IEnumerable<Payment> Payments { get; set; }
}

Right now I am doing so by this function:

private IEnumerable<CustomerInfo> _GetCustomerInfo(
    IEnumerable<Payment> payments, IEnumerable<Invoice> invoices,
    IEnumerable<Order> orders)
{
    var invoicesGroupdByCustomers = invoices.GroupBy(x => x.CustomerId);
    var ordersGroupdByCustomers = orders.GroupBy(x => x.CustomerId);
    var paymentsGroupdByCustomers = payments.GroupBy(x => x.CustomerId);

    var result = new List<CustomerInfo>();
    foreach (var group in invoicesGroupdByCustomers)
        result.Add(new CustomerInfo(group.Key, 
            ordersGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key), 
            group,
            paymentsGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key)));

    foreach (var group in ordersGroupdByCustomers)
        if (!result.Any(x => x.CustomerId == group.Key))
            result.Add(new CustomerInfo(group.Key, 
                group,
                invoicesGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key),
                paymentsGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key)));

    foreach (var group in paymentsGroupdByCustomers)
        if (!result.Any(x => x.CustomerId == group.Key))
            result.Add(new CustomerInfo(group.Key,
                ordersGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key),
                invoicesGroupdByCustomers.FirstOrDefault(x => x.Key == group.Key),
                group));

    return result;
}

May be there is better approach?

Update: Here is sample of calculations where I use these transactions collections:

private Dictionary<Guid, decimal> _CalculateSales(
    IEnumerable<CustomerInfo> customersSalesInfo)
{
    var result = new Dictionary<Guid, decimal>();

    foreach (var customerInfo in customersSalesInfo)
    {
        var sales = customerInfo.Invoices.Sum(x => x.Header.SubTotalAmt) +
            customerInfo.Orders.Sum(x => x.Header.SubTotalAmt) -
            customerInfo.Payments.Sum(x => x.Header.SubTotalAmt);
        result.Add(customerInfo.CustomerId, sales);
    }

    return result;
}
share|improve this question

5 Answers 5

up vote 1 down vote accepted

If you're developing a production solution I would rather think how to avoid loading all the customers/invoices/etc at first place. If that's the case then please describe the use cases in which Customerinfo is going to be used, how do you get invoices/orders/payments collections, and is there a chance to leverage ORM capabilities (assuming that you get the data via some sort of ORM) to load these collections for customers?

But if we are just talking about programming exercise then I would probably go with the following solution (influenced by RavenDB's map/reduce approach):

IEnumerable<CustomerInfo> infos = (
    from invoice in invoices
    select new { invoice.CustomerId, Invoice = invoice, Order = null, Payment = null }
    ).Concat(
        from order in orders
        select new { order.CustomerId, Invoice = null, Order = order, Payment = null }
    ).Concat(
        from payment in payments
        select new { payment.CustomerId, Invoice = null, Order = null, Payment = payment }
    ).GroupBy(x=>x.CustomerId, (key, group) => new CustomerInfo(key, 
        group.Select(x => x.Invoice).Where(i => i != null),
        group.Select(x => x.Order).Where(o => o != null),
        group.Select(x => x.Payment).Where(p => p != null));

I can't test this code right now, you might have to explicitly specify types in anonymous objects.

Update: Based on your usage of CustomerInfo you might not actually need it at all (if that's all you do with it) :). I would rather try to reduce the amount of data straight away, thus improving performance and reducing memory usage:

private Dictionary<Guid, decimal> _CalculateSales(
    IEnumerable<Payment> payments, IEnumerable<Invoice> invoices,
    IEnumerable<Order> orders)
{
    var result = (
        from invoice in invoices
        select new { invoice.CustomerId, Amount = invoice.Header.SubTotalAmt}
        ).Concat(
            from order in orders
            select new { order.CustomerId, Amount = order.Header.SubTotalAmt}
        ).Concat(
            from payment in payments
            select new { payment.CustomerId, Amount = -payment.Header.SubTotalAmt}
        ).GroupBy(x => x.CustomerId)
        .ToDictionary(g => g.Key, g => g.Sum(x => x.Amount));
    return result;
}

The benefit of this solution comparing to original code is that it iterates over collections only once, and the only Dictionary created is the actual result.

share|improve this answer
    
I really need to load all transactions. The task is to calculate sales for all customer for some period of time. So, first I'd get list of all transactions for this time, then to group them by customer, then calculate sales for each customer. Thanks for nice code. –  Seekeer Nov 16 '12 at 5:33
    
In this case I would rather use DB capabilities to groups transactions by CustomerId, unless you are doing some complex calculations there. –  almaz Nov 16 '12 at 5:42
    
Can you provide a sample of calculations you're doing? –  almaz Nov 16 '12 at 5:48
    
problem is that I am getting transaction not from DB, but from really dumb web service, it can't do grouping. I've updated question with a sample of calculations. –  Seekeer Nov 16 '12 at 6:16
    
I've updated my response according to your update –  almaz Nov 16 '12 at 6:44

If they all implemented a common interface then you could simplify it.

 public interface ICustomerItem
    {
        Guid CustomerId {get;}
     }

Concat the the items together

var items = invoices.Concat(payments).Concat(orders);

Then project them into your class

 items
.GroupBy(x => x.CustomerId)
.Select(g => 
          new CustomerInfo(g.Key, 
                 g.OfType<Order>(), 
                 g.OfType<Payment>(),
                 g.OfType<Invoice>())
share|improve this answer
    
Thanks for "Concat", I've forgot about it. But in my case these classes aren't implementing common interface(in fact, they are implementing, but there is no such property as "CustomerId"). And these classes are from 3d party dll, which I couldn't change. –  Seekeer Nov 15 '12 at 14:25
    
You could use DLINQ or create a wrapper classes for each item that implements the interface. i.e. OrderWrapper, PaymentWrapper and InvoiceWrapper. When you concat them, first project each item into the wrapper. –  Adam Mills Nov 15 '12 at 15:22

I think a better way would be to extract all of the ids first and then create your result based on that:

private IEnumerable<CustomerInfo> GetCustomerInfo(
    IEnumerable<Payment> payments, IEnumerable<Invoice> invoices,
    IEnumerable<Order> orders)
{
    // if the parameters were ILists, this wouldn't be necessary
    payments = payments.ToArray();
    invoices = invoices.ToArray();
    orders = orders.ToArray();

    var customerIds = payments.Select(p => p.CustomerId)
        .Concat(invoices.Select(i => i.CustomerId))
        .Concat(orders.Select(o => o.CustomerId))
        .Distinct()
        .ToArray();

    // using ToLookup() means we can efficiently retrieve items based on id
    var paymentsLookup = payments.ToLookup(p => p.CustomerId);
    var invoicesLookup = invoices.ToLookup(i => i.CustomerId);
    var ordersLookup = orders.ToLookup(o => o.CustomerId);

    return customerIds.Select(
        id => new CustomerInfo(id, ordersLookup[id], invoicesLookup[id], paymentsLookup[id]));
}
share|improve this answer
    
Thanks, I finally come to same approach by myself. –  Seekeer Nov 19 '12 at 8:54

Maybe I'm missing something but it seems that your code have strange behavior. If orders and payments collections have items with same CustomerId as in invoices collection then by checking

if (!result.Any(x => x.CustomerId == group.Key))

you will lose all of them and result collection will contain one CustomerInfo element per customer with some invoices and orders == null, payments == null.

If this behavior is ok I would recommend using Dictionary to avoid multiple list scanning with Any()

private IEnumerable<CustomerInfo> _GetCustomerInfo(
        IEnumerable<Payment> payments,
        IEnumerable<Invoice> invoices,
        IEnumerable<Order> orders)
    {
        if (payments == null) throw new ArgumentNullException("payments");
        if (invoices == null) throw new ArgumentNullException("invoices");
        if (orders == null) throw new ArgumentNullException("orders");

        var invoicesGroupdByCustomers = invoices.GroupBy(x => x.CustomerId);
        var ordersGroupdByCustomers = orders.GroupBy(x => x.CustomerId);
        var paymentsGroupdByCustomers = payments.GroupBy(x => x.CustomerId);

        var result = new Dictionary<Guid, CustomerInfo>();
        foreach (var group in invoicesGroupdByCustomers)
            result.Add(group.Key, new CustomerInfo(group.Key, null, group, null));

        foreach (var group in ordersGroupdByCustomers)
            if (!result.ContainsKey(group.Key))
                result.Add(group.Key, new CustomerInfo(group.Key, group, null, null));

        foreach (var group in paymentsGroupdByCustomers)
            if (!result.ContainsKey(group.Key))
                result.Add(group.Key, new CustomerInfo(group.Key, null, null, group));

        return result.Values;
    }

Hope this helps

share|improve this answer
    
Thanks for pointing out my error. Nope thats not what I want, I've fixed code in question. –  Seekeer Nov 16 '12 at 5:27
    
-1: so instead of losing orders and payments for customer that have invoices (and payments for customers that already have orders), you make the code throw an exception on the .Add() method? That hardly seems an improvement. –  ANeves Nov 16 '12 at 8:54

You could take your original approach and use .ToList()

var invoicesGroupdByCustomers = invoices.GroupBy(x => x.CustomerId).ToList();
var ordersGroupdByCustomers = orders.GroupBy(x => x.CustomerId).ToList();
var paymentsGroupdByCustomers = payments.GroupBy(x => x.CustomerId).ToList();

Just another option to be aware of. I hope this helps.

share|improve this answer
1  
-1. How is this helpful? What does it improve? How could this be used to make the original code better? That said, please don't get upset with the downvotes, it's a natural and neutral thing... we value your participation, welcome to the community! –  ANeves Nov 15 '12 at 18:24

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.