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 = pendingEnt开发者_运维技巧ries.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());
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.
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.
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
I'd rewrite it as more standard while
method. And you've forgot that IEnumerator<T>
implements IDisposable
, so you should dispose it.
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
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.
精彩评论