4
\$\begingroup\$

I'm using an ObservableCollection in a WPF app. The source of the data doesn't provide changes. It provides snapshots instead. The method I wrote to update the ObservableCollection works but it seems inelegant. Is there a better way to do this?

    public static void UpdateMarketOrderList(
                          ObservableCollection<MarketOrder> oldList, 
                          List<MarketOrder> newList)
    {
        int oi = 0; // old list iterator
        int ni = 0; // new list iterator

        while (ni < newList.Count)
        {
            if (oi == oldList.Count)
            {
                oldList.Insert(oi, newList[ni]);    
            } 
            else if (oldList[oi].Price == newList[ni].Price)
            {
                if (oldList[oi].Quantity != newList[ni].Quantity)
                {
                    oldList[oi].Quantity = newList[ni].Quantity;
                }
                ++oi;
                ++ni;
            }
            else if (oldList[oi].Price < newList[ni].Price)
            {
                oldList.RemoveAt(oi);
            }
            else if (oldList[oi].Price > newList[ni].Price)
            {
                oldList.Insert(oi, newList[ni]);
            }
        }

        while (oldList.Count > ni)
        {
            oldList.RemoveAt(ni);
        }
    }

If it matters, the definition MarketOrder can be considered to be this:

public class MarketOrder 
{
    public decimal Price { get; set; }
    public decimal Quantity { get; set; }
}
\$\endgroup\$
2
  • \$\begingroup\$ Is it assumed that lists are sorted by Price and there are no duplicates per Price? \$\endgroup\$
    – Snowbear
    Jun 30, 2011 at 19:33
  • \$\begingroup\$ Snowbear: Yes. I do an .OrderBy(x => x.Price).ToList() before calling. I should probably do that in the method itself. \$\endgroup\$
    – Jere.Jones
    Jun 30, 2011 at 19:50

1 Answer 1

3
\$\begingroup\$

After studying your code snippet for a bit here's what I came up with.

I wouldn't call them iterators. They're really acting more as indexing operators that doubles as a loop counter. Furthermore, they're always incremented in sync and they have the exact same initial value.

int oi = 0; // old list iterator
int ni = 0; // new list iterator

So I would replace with simply:

int i = 0;

The second loop for trimming the extra elements:

while (oldList.Count > ni)
{
  oldList.RemoveAt(ni);
}

The post-condition for i after the first loop has to be equal to newList.Count otherwise we would still be looping the first while. Thusly, changing the condition to this is clearer:

while (oldList.Count > newList.Count)

After tracing your UpdateMarketOrderList method, the discernible end effect, from what I can tell, just seems to be a replacement of the oldList with the newList. If that's really the desired end consequence of running this function, why not simplify it, succinctly to this instead?

public static void UpdateMarketOrderList( ObservableCollection<MarketOrder> oldList, 
                                          List<MarketOrder> newList )
{
    oldList.Clear();

    for(int i = 0; i < newList.Count; ++i)
    {
        oldList.Add(newList[i]);
    }
}

Then again you didn't specify in your question what the pre/post-conditions are for UpdateMarketOrderList, which is something that does matter.


After reading OP's comment that fired change events are important, I would refactor the main loop like this:

    while (i < newList.Count)
    {
        if (i == oldList.Count ||
            oldList[i].Price > newList[i].Price)
        {
            oldList.Insert(i, newList[ni]);    
        }
        else if (oldList[i].Price < newList[i].Price)
        {
            oldList.RemoveAt(i);
            continue;
        }
        else if (oldList[i].Quantity != newList[i].Quantity) 
              // && oldList[i].Price == newList[i].Price
        {
            Debug.Assert(oldList[i].Price == newList[i].Price);
            oldList[i].Quantity = newList[i].Quantity;
        }

        ++i;
    }

With this refactor you decrease the indent level and tighten the loop a bit while preserving existing behavior. The key thing to realize is that RemoveAt doesn't increment the loop counter. Your previous code did that in a slightly less direct manner.

My gut feeling is that this could be refactored further still but without a good idea of the expected side-effects and other requirements for this method, it will be difficult to refactor this safely.

\$\endgroup\$
2
  • \$\begingroup\$ Clearing and then adding elements isn't acceptable because the change information is lost. Notice that it is an ObservableCollection and changes fire events. That said, the first half of your answer is gold. Thanks! \$\endgroup\$
    – Jere.Jones
    Jul 2, 2011 at 8:44
  • \$\begingroup\$ @Jere I remember reading about the fire events in msdn but didn't realize at the time it was important for your scenario. I've edited my answer with more info. \$\endgroup\$
    – greatwolf
    Jul 13, 2011 at 1:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.