Refactor Code

Performing Action on all Items of a Collection
by azamsharp on 6/23/2008 3:17:09 PM
Sometimes I am in a situation that I need to perform a certain action on all the items of the list. It is easy if the collection is in the form of List. Even if it is in the form IList or IEnumerable it is easy to simply convert it to List. I am looking for a better way to perform this action without doing a conversion. Off course I can run a foreach loop or use the Select method to do it but I am looking for a simple compact solution.
Func<Customer, Customer> funcDiscount = new Func<Customer, Customer>(GiveCustomerDiscount);

IList<Customer> customers = new List<Customer>()
{ new Customer() { Name="Mohammad Azam" }, new Customer() { Name = "John Doe" }
, new Customer() { Name = "Patrick" } , new Customer() { Name = "Mary Kate" }
};

// solution one
customers.ToList<Customer>().ForEach(delegate(Customer customer)
{
customer.Discount = 10;
});

// solution two:
IEnumerable<Customer> cs = customers.Select<Customer, Customer>(funcDiscount);

public static Customer GiveCustomerDiscount(Customer customer)
{
customer.Discount = 10;
return customer;
}
Refactor it!

Both solutions give me the creeps. In solution one, you convert your collection to a List, simply to call ForEach on it. I think you're making things look more difficult than they really are. The second solution is actually worse. I think you are misusing the Select method. When I see some code with a call to Select (like your customers.Select), I simply wouldn't expect the call to have any side effects. But it actually does. I'd opt for the plain old foreach approach. It's the fastest, most readable and most maintainable code you'll get.
by .NET Junkie on 6/26/2008 10:13:58 AM
// solution three
foreach (Customer customer in customers)
{
customer.Discount = 10;
}

Thanks .NET Junkie! I agree! Also, it would be great if you could discuss some side effects of using the Select method.
by azamsharp on 6/26/2008 10:55:36 AM

The Select method is one of the new extension methods that allow LINQ to work. The second solution, using the Select method could be rewritten to a LINQ query. Therefore: IEnumerable cs = customers.Select(funcDiscount); is completely equivalent to the following: var cs = from customer in customers select funcDiscount; So there are a couple of assumptions people (like me) will make when seeing a Select method call: 1. The Select is used for queries. 2. Queries are side effect free. A side effect is a (unexpected) modification of some state in addition to returning a value. In our case, the funcDiscount method produces a side effect by changing every customer object that goes through it. In out normal imperative programming style (thus simply calling funcDiscount) this is no problem. But LINQ is a functional approach en a query isn’t expected to change state. A reviewer (or someone that must bug fix your code) simply won’t expect this when looking at a query. Neither would you expect the SQL query “SELECT * from customer” to change the state of your database. But when you look at it more closely, you’ll see that it only gets worse. Because of deferred execution, the “customers.Select(funcDiscount)” statement actually doesn't do anything. It doesn’t change any state! It's only after the IEnumerable is iterated that it changes the state. So you will always have to foreach over it, to change the state). Foreaching over it twice will actually change state twice (the effect of this is easier to spot when the method increases the Discount by 10). I hope this makes sense.
by .NET Junkie on 6/26/2008 12:29:48 PM

Thanks .NET Junkie! Keep these good refactorings coming!
by azamsharp on 6/26/2008 3:09:06 PM

Please log in to refactor the code! Login