Here is some poorly written code that I had to review today and modify (i.e refactor). I have removed the domain of the code and renamed the variables, so that it remains anonymous, but the just of the code is still intact.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 |
var itemCount = 0; if (users.ShoppingCart != null && user.ShoppingCart > 0) { foreach (var cart in user.ShoppingCart) { if (cart.Items != null && cart.Items.Count > 0) { foreach (var item in cart.Items) { itemCount++; } } } } |
Lets go through the inner loop of the code first.
First the loop is really unneccesary one can find out how many items there are using the
count of if since it is an IEnumerable
Thus it becomes
|
1 2 3 4 5 6 7 8 9 10 11 |
var itemCount = 0; if (users.ShoppingCart != null && user.ShoppingCart > 0) { foreach (var cart in user.ShoppingCart) { if (cart.Items != null && cart.Items.Count > 0) { itemCount += cart.Items.Count(); } } } |
Now we go to the outer foreach we can put a Where clause to remove the if statement we don’t really need to check for the “0” condition since 1 + 0 is still 1 so why filter for those.
|
1 2 3 4 |
foreach (var cart in user.ShoppingCart.Where(x => x.Items != null) { itemCount += cart.Items.Count(); } |
Finally lets remove the entire foreach loop and the count if statement. Since if we have 0 items this code will still not fail.
|
1 2 3 4 |
if (users.ShoppingCart != null) { itemCount = users.ShoppingCart.Where(x => x.Items != null).Sum(y => y.Items.Count()); } |
Now we have replaced all this code with a linq query that would tell us how many items there are with one line of linq. I believe this makes is easier on the eye and readability of it rather than looping and branching of the code. The more branches you have the harder it is to read code.
