Make method fewer lines of code

Question

I have a method like below, is there any way to refactor, cleaner way, so that i can make it in fewer lines of code for e.g removal of if / for loops someting like that

public void CheckProductExistThenAddToCart(CartItem item) { if (CartItems.Count == 0) AddToCart(item); bool itemFound = false; foreach (var cartItem in CartItems) { if (cartItem.ProductId.Equals(item.ProductId)) { itemFound = true; cartItem.Qty += item.Qty; break; } } if (!itemFound) { AddToCart(item); } }

This looks like it would be better placed on codereview.stackexchange.com — Gjeltema
– Gjeltema, Commented Aug 31, 2013 at 23:05
Using LINQ, as given in the answer by Reed Copsey, would reduce your lines of code significantly. Do be advised though, that fewer lines of code can actually reduce your code readability/maintainability quite badly. In other words: If one line of code would need documentation so one would understand what it does, while two lines of code would be self-explanatory, go for the two lines of code. You are not paid for "most functionality per line." You are paid for correct, readable and maintainable code. — Jan Dörrenhaus
– Jan Dörrenhaus, Commented Aug 31, 2013 at 23:10
@JanDoerrenhaus True - though I'd argue that my version is far more readable and maintainable than the original... — Reed Copsey
– Reed Copsey, Commented Aug 31, 2013 at 23:29
@ReedCopsey Oh, it is (as long as you are fluent in LINQ, which can be safely assumed nowadays). This was more of a general observation regarding the question the OP asked. The question should always be "How do I make my code more readable and maintainable", not "How do I reduce my number of lines." — Jan Dörrenhaus
– Jan Dörrenhaus, Commented Sep 1, 2013 at 22:02

Reed Copsey · Accepted Answer · 2013-08-31 23:07:08Z

You could use LINQ:

public void CheckProductExistThenAddToCart(CartItem item) { var existingItem = CartItems.FirstOrDefault(ci => ci.ProductID == item.ProductId); if (existingItem == null) CartItems.Add(item); else existingItem.Qty += item.Qty; }

BartoszKP · Accepted Answer · 2013-08-31 23:13:20Z

You can use SingleOrDefault if having unique items (in the context of ProductId) should be assured. If it is possible to have more than one and you want to ignore this fact, then change to FirstOrDefault. I find Single better as it states intent explicitly here.

public void CheckProductExistThenAddToCart(CartItem item) { var existingItem = CartItems .SingleOrDefault(i => i.ProductId.Equals(item.ProductId)); if (existingItem == null) { AddToCart(item); } else { existingItem.Qty += item.Qty; } }

Jesko R. · Accepted Answer · 2013-08-31 23:10:05Z

4

In order to shorten this function you can consider using a

Dictionary<ProductId, CartItem> dict;

Then instead of looping through the cart just use

if (dict.ContainsKey(productId)) { // add qty } else { // add item to cart }

answered Aug 31, 2013 at 23:10

Jesko R.

82710 silver badges21 bronze badges

2 Comments

Reed Copsey Over a year ago

A dictionary makes this method far simpler, but may make the other processing more complex. For example, a dictionary won't preserve order, and most people will complain if adding an item to your cart shuffles the order of the items, etc.

Jesko R. Over a year ago

Sure, but usually I do not use plain collections for this purpose. I will start off with a collection and then put it in a class that will help me organize the issue. e.g. you can then add an array that will help with ordering the cart once you need to put it on screen.

Tim Schmelter · Accepted Answer · 2013-08-31 23:14:39Z

First, there is a bug because you are not returning after you have added the missing item. Hence you add Qty to the same item you have added one moment before, hence it's value is doubled.

So instead of:

public void CheckProductExistThenAddToCart(CartItem item) { if (CartItems.Count == 0) AddToCart(item); // missing return bool itemFound = false; foreach (var cartItem in CartItems) { if (cartItem.ProductId.Equals(item.ProductId)) { itemFound = true; // ypu will find the same item you have justb added // ... causes this bug and is less efficient cartItem.Qty += item.Qty; ...

i would do (also simplified with Linq):

public void CheckProductExistThenAddToCart(CartItem item) { if (CartItems.Count == 0) { AddToCart(item); return; } CartItem oldItem = CartItems.FirstOrDefault(ci => ci.ProductId == item.ProductId); if(oldItem == null) AddToCart(item); else oldItem.Qty += item.Qty; }

Actually the first if statement is redundant with FirstOrDefault. However, i have kept it to show the missing return statement, the reason for the hidden bug.

Collectives™ on Stack Overflow

Make method fewer lines of code

4 Answers 4

1 Comment

1 Comment

2 Comments

1 Comment

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

1 Comment

2 Comments

1 Comment

Related