CODEWITHSUNDEEP
c#foreachdo-while
neontapir
neontapir

Reputation: 4736

Can I rewrite this do-while loop as a foreach loop?

Is there a way to write this code more elegantly with a foreach loop? The "create a new entry" logic is thwarting me, because it needs to execute even if pendingEntries contains no items.

ItemDto itemToAdd; // an input parameter to the method
IEnumerator<Item> pendingEntries = existingPendingItems.GetEnumerator();
pendingEntries.MoveNext();
do // foreach entry
{
  Item entry = pendingEntries.Current;
  if (entry != null) // fold the itemToAdd into the existing entry
  {
    entry.Quantity += itemToAdd.Quantity; // amongst other things
  }
  else // create a new entry
  {
    entry = Mapper.Map<ItemDto, Item>(itemToAdd);
  }
  Save(entry);
} while (pendingEntries.MoveNext());

Upvotes: 3

Views: 798

Answers (6)

Mykroft
Mykroft

Reputation: 13435

Personally I'd suggest something like this:

ItemDto itemToAdd; // an input parameter to the method
if (existingPendingItems.Any())
{
    foreach(Item entry in existingPendingItems)
    {
        entry.Quantity += itemToAdd.Quantity    
        Save(entry);
    }
}
else
{
    entry = Mapper.Map<ItemDto, Item>(itemToAdd);
    Save(entry);
}

I think this makes the intent of the code much clearer.

EDIT: Changed count to any as per suggestion. Also fixed the add quantity logic

Upvotes: 1

Paul Ruane
Paul Ruane

Reputation: 38600

foreach (Item entry in existingPendingItems.DefaultIfEmpty())
{
    Item entryToSave;

    if (entry != null) // fold the itemToAdd into the existing entry
    {
        entry.Quantity += itemToAdd.Quantity; // amongst other things

        entryToSave = entry;
    }
    else // create a new entry
    {
        entryToSave = Mapper.Map<ItemDto, Item>(itemToAdd);
    }

    Save(entryToSave);
}

The key is the Enumerable.DefaultIfEmpty() call — this will return a sequence with a default (Item) item if the sequence is empty. This will be null for a reference type.

Edit: fixed bug mentioned by neotapir.

Upvotes: 2

Julien Lebosquain
Julien Lebosquain

Reputation: 41243

This should be rewritten. I don't know what kind of collection you're using, but Current is undefined in your case since MoveNext could have returned false. As stated in the documentation:

Current is undefined under any of the following conditions: The last call to MoveNext returned false, which indicates the end of the collection.

Here is how I would rewrite it:

bool isEmpty = true;
foreach (Item entry in existingPendingItems)
{
  ProcessEntry(entry, itemToAdd);
  isEmpty = false;
}
if (isEmpty)
{
  ProcessEntry(null, itemToAdd);
}
  • ProcessEntry contains the logic for a single entry, and is easily unit testable.
  • The algorithm is cleared to read.
  • The enumerable is still only enumerated once.

Upvotes: 4

Jay
Jay

Reputation: 57959

var pendingEntries = existingPendingItems.Any()
    ? existingPendingItems
    : new List<Item> { Mapper.Map<ItemDto, Item>(itemToAdd) };

foreach (var entry in pendingEntries)
{
    entry.Quantity += itemToAdd.Quantity; // amongst other things
    Save(entry);
}

The idea here is that you set yourself up for success before iterating. What are you going to iterate over? Either the existing entries, if there are any, or just a new entry otherwise.

By handling this up front, so you know you've got something with which to work, your loop stays very clean.

Upvotes: 0

Grant
Grant

Reputation: 684

foreach( Item entry in pendingEntries.Current)
{
    if( entry != null)
        entry.Quantity += itemToAdd.Quantity;
    else
       entry = Mapper.Map<ItemDto, Item>(itemToAdd);
    Save(entry)
}

cant exactly test it without the items

Upvotes: 0

Ivan Danilov
Ivan Danilov

Reputation: 14777

I'd rewrite it as more standard while method. And you've forgot that IEnumerator<T> implements IDisposable, so you should dispose it.

Upvotes: 0

Related Questions