CODEWITHSUNDEEP
c#.netlinqlambda
magoverflow
magoverflow

Reputation: 23

Why Doesn't My Anonymous Method Work in a Loop?

This function is supposed to set descending order numbers on an IEnumerable<Order>, but it doesn't work. Can anyone tell me what's wrong with it? 

private void orderNumberSetter(IEnumerable<Order> orders)
{
    var i = 0;            

    Action<Order, int> setOrderNumber = (Order o, int count) =>
    {
        o.orderNumber = i--;
    };

    var orderArray = orders.ToArray();
    for (i = 0; i < orders.Count(); i++)
    {
        var order = orderArray[i];
        setOrderNumber(order, i);
    }            
}

Upvotes: 2

Views: 190

Answers (4)

BrokenGlass
BrokenGlass

Reputation: 161012

You are re-using i as loop variable and i gets modified in your setOrderNumber lambda - don't modify i - it's unclear what you meant to do, maybe the following:

Action<Order, int> setOrderNumber = (Order o, int count) =>
{
    o.orderNumber = count;
};

If the above is the case you could have achieved that much, much easier though, your code seems unnecessarily complex, i.e:

for (i = 0; i < orderArray.Length; i++)
{
    orderArray[i].orderNumber = i;
}

or even simpler without having to create an array at all:

int orderNum = 0;
foreach(var order in orders)
{
   order.orderNumber = orderNum++;
}

Edit:

To set descending order numbers, you can determine the number of orders first then go backwards from there:

int orderNum = orders.Count();
foreach(var order in orders)
{
   order.orderNumber = orderNum--;
}

Above would produce one based order numbers in descending order. Another approach, more intuitive and probably easier to maintain is to just walk the enumeration in reverse order:

int orderNum = 0;
foreach(var order in orders.Reverse())
{
   order.orderNumber = orderNum++;
}

Upvotes: 9

Mike McFarland
Mike McFarland

Reputation: 657

Though its hard to tell what your trying to do, its a good bet that you didn't mean to keep referring to the same variable i, which is whats causing an infinite loop. heres another example of what I believe you wanted

IEnumerable<Order> reversed = orders.ToArray(); //To avoid editing the original
reversed.Reverse();
int orderNumber = 0; 
foreach (Order order in reversed)
{
    order.orderNumber = orderNumber++;
}

I suggest editing your title. Your title describes your question, and I'm sure you didn't want a Broken C# function, since you already had one :P. Its also good to describe what your code to do in the post thoroughly, including what your expected results are, and how your current example doesn't meet them. Don't let your non working example alone explain what you want, It only showed us an example of what you didn't want.

Upvotes: 2

Francois
Francois

Reputation: 330

I would try this code instead that decrements i while it enumerates through the array

private void orderNumberSetter(IEnumerable<Order> orders)
{
    int i = orders.Count();
    foreach (Order order in orders.ToArray())
    {
        order.orderNumber = --i;
    }            
}

Upvotes: 2

tobias86
tobias86

Reputation: 5029

I agree with BrokenGlass, you are running into an infinite loop.

You could achieve the same thing using foreach:

private void orderNumberSetter(IEnumerable<Order> orders)
{
    var count = orders.Count();

    orders.ToList().ForEach(o =>
    {
         o.orderNumber = count--;
    });
}

Upvotes: 2

Related Questions