Geeks With Blogs
Chris Falter .NET Design and Best Practices

A few years ago I listened in fascination to a radio interview with a highway patrol officer who was describing how she conducted field sobriety tests.  In addition to the typical tests of walking in a straight line and of standing on one foot, she would ask the erratic driver to recite the alphabet.  One driver slurred, "A.  Is that good enough?"  "That's all the alphabet you can recite?" she responded.  "Yep," he answered.  She booked him.  She would also ask drivers to count backwards from 100.  If a driver skipped around or had to think hard, his mental facilities were not sharp enough to permit him to drive safely.

Counting backwards can also come in handy when you're performing a loop iteration.  For years we had some data access code that looped through a DataRowCollection in order to commit changes to a DB2 database:

public void SaveRows(DataTable tbl)
{
    
string spName = null;
    
for (int i = 0; i < tbl.Rows.Count; i++)
    {
        DataRow row = tbl.Rows[i];
        DataRowState rowState = row.RowState;
        
if ((rowState & DataRowState.Unchanged) > 0)
            
continue;
        
switch (row.RowState)
        {
            
case DataRowState.Added:
                spName =
"SPI" + tbl.TableName;
                
break;
            
case DataRowState.Modified:
                spName =
"SPU" + tbl.TableName;
                
break;
            
case DataRowState.Deleted:
                spName =
"SPD" + tbl.TableName;
                
break;
        }
        
try
        {
            IDbDataParameter[] parms = DataHelperParameterCache.GetSpParameterSet(DataHelper.ConnectionString, spName);
            CallStoredProc(spName, parms, row);
        }
        
catch (Exception ex)
        {
            
if (tx != null)
            {
                tx.Rollback();
                rolledBack =
true;
            }
            
throw new BaseApplicationException("Fatal Exception while running stored proc " + spName, ex);
        }
    }
}

Then after we had called this function on all the DataTables in the DataSet, we called DataSet.AcceptChanges().  This code generally worked well.

Then one day we realized that by not using a IDbTransaction (we left it null) and calling DataSet.AcceptChanges() at the end, we would leave our DataSet in a state inconsistent with the data store if a failure occurred part way through the process of saving tables and rows.  For example, if a row had been inserted to the data store, but AcceptChanges() had not been called, then its RowState was still DataRowState.Added.  And the next time a user tried to save his work, our system would try to insert an additional row, rather than updating the row that was already in the data store.  So we decided to call DataRow.AcceptChanges() after each row operation (after the call to CallStoredProc in the above code).

A few days later, our testers reported that the last of 2 delete operations would no longer commit to the data store.  What?  How could adopting a new strategy for accepting changes cause that kind of bug?

Well, what happens when you call DataRow.AcceptChanges() when the RowState is DataRowState.Deleted?  Poof!  It vanishes into thin air.  As a result, the next row would get skipped over in the loop iteration, since its ordinal had shifted back by one. 

Allow me to illustrate: you've got 2 rows, and you're going through the loop.  The first time you enter the loop body, i is 0 and you're working with Rows[0], the first row.  Before you finish the loop, you call DataRow.AcceptChanges(), and the row disappears in a cloud of smoke, alakazam!  Now the row that used to be at Rows[1] shifts back to Rows[0], and Rows.Count == 1.  And we're at the end the loop, so we increment i to 1 and do the test:

i < tbl.Rows.Count

This statement is false, so we exit the for loop.  Thus the row that shifted from index 1 to index 0 (when its sibling vanished) never got processed.

My first thought on fixing the problem was to use a foreach loop and let it figure out how to deal with the deletion for me.  However, the DataRowCollection quite correctly threw an InvalidOperationException when we tried it, and I can't blame the class designers for the behavior.  Remember, the iterator is supposed to keep track of the Current and the Next member of the collection as you traverse it.  But if you delete the Current member, where should Current point to?  Can it point to the member of the collection immediately following the deleted member?  Not really; Current and Next would then be referring to the same member.  And if the iterator shifted the Next by one to avoid the clash, the same way it shifted Current by one, we might skip over the collection member that immediately follows the deleted member when the iterator's MoveNext method is called.  As the .NET Framework documentation states:

The foreach statement is used to iterate through the collection to get the desired information, but should not be used to change the contents of the collection to avoid unpredictable side effects. [emphasis mine]

My colleague Kyle then came up with a terrific idea: just count backwards!  Start at the last member and iterate back to the first; and if a member is deleted, the loop still operates correctly.   So now our code looks like this:

for (int i = tbl.Rows.Count - 1; i >= 0; i--)
{
    
// everything else is the same as before
}

You might not think the great programmers grow up in Appalachian hill country, but Kentucky Kyle has been full of pleasant surprises for us ever since we were astute enough to hire him.  Or maybe they just have to deal with field sobriety tests a lot more in the Kentucky hills.  Whatever the background, my hat is off to Kyle for his excellent idea.

 

Posted on Thursday, November 8, 2007 5:44 AM .NET Gotchas , Coding Practices and Design Patterns | Back to top


Comments on this post: Of Sobriety Tests and Loop Iteration

No comments posted yet.
Your comment:
 (will show your gravatar)


Copyright © Chris Falter | Powered by: GeeksWithBlogs.net