Is there a better way? While loops and continues

Question

There are many functions within the code I am maintaining which have what could be described as boilerplate heavy. Here is the boilerplate pattern which is repeated ad nausea throughout the application when handling DB I/O with a cursor:

if( !RowValue( row, m_InferredTable->YearColumn(), m_InferredTable->YearName(), m_InferredTable->TableName(), value )
  || !IsValidValue( value ) )
 {
  GetNextRow( cursor, m_InferredTable );

  continue;
 }
 else
 {
  value.ChangeType(VT_INT);
  element.SetYear( value.intVal );
 }

The thing is not all of these statements like this deal with ints, this "element" object, the "year" column, etc. I've been asked to look at condensing it even further than it already is and I can't think of a way to do it. I keep tripping over the continue statement and the accessors of the various classes.

Edit: Thanks to all those that commented. This is why I love this site. Here is an expanded view:

while( row != NULL )
{
 Element element;
 value.ClearToZero();
 if( !GetRowValue( row, m_InferredTable->DayColumn(), m_InferredTable->DayName(), m_InferredTable->TableName(), value )
  || !IsValidValue( value ) )
 {
  GetNextRow( cursor, m_InferredTable );

  continue;
 }
 else
 {
  value.ChangeType(VT_INT);
  element.SetDay( value.intVal );
 }

And things continue onward like this. Not all values taken from a "row" are ints. The last clause in the while loop is "GetNextRow."

Answer 1

Why not invert your if-test?

   if (RowValue(row, m_InferredTable->YearColumn(), m_InferredTable->YearName(),  m_InferredTable->TableName(), value ) 
     && IsValidValue( value ))
   {
     value.ChangeType(VT_INT);
     element.SetYear( value.intVal );
   }
   else
   {
     GetNextRow( cursor, m_InferredTable );
   }

Answer 2

My instinctual approach is to build a polymorphic approach here, where you eventually wind up doing something like(modulo your language and exact logic):

db_cursor cursor;

while(cursor.valid())
{
  if(cursor.data.valid())
  { 
     process();
  }

  cursor.next();
}

db_cursor would be a base class that your different table type classes inherit from, and the child classes would implement the validity functions.

Answer 3

Move it into a template function, templated on the element type (e.g. integer), which you can call over and over. Vary the behavior per data type with a trait template.

template <typename T> struct ElemTrait<T> {};
template <> struct ElemTrait<int> {
    static inline void set(Val &value, Elem &element) {
     value.ChangeType(VT_INT);
     element.SetYear(value.intVal);
    }
};
// template <> struct ElemTrait<float> { ... };

template <typename T>
void do_stuff( ... ) {
    // ...

    if (!RowValue(row,
        m_InferredTable->YearColumn(),
        m_InferredTable->YearName(),
        m_InferredTable->TableName(), value)
        || !IsValidValue(value)
    ) {
        GetNextRow(cursor, m_InferredTable);
        continue;
    } else {
        ElemTrait<T>::set(value, element);
    }

    // ...
}

Answer 4

You can take out all the GetNextRow calls and the else clauses:

for (row = GetFirstRow () ; row != null ; GetNextRow ())
{
    Element element;
    value.ClearToZero();
    if( !GetRowValue( row, m_InferredTable->DayColumn(), m_MetInferredOutTable->DayName(), m_MetInferredOutTable->TableName(), value )
            || !IsValidValue( value ) )
    {
      continue;
    }

    value.ChangeType(VT_INT);
    element.SetDay( value.intVal );
}

Answer 5

Okay, from what you've said, you have a structure something like this:

while (row!=NULL)  {
    if (!x) {
        GetNextRow();
        continue;
   }
   else {
       SetType(someType);
       SetValue(someValue);
   }
   if (!y) {
       GetNextRow();
       continue;
   }
   else {
       SetType(SomeOtherType);
       SetValue(someOtherValue);
   }
// ...

   GetNextRow();   
}

If that really is correct, I'd get rid of all the GetNextRow calls except for the last one. I'd then structure the code something like:

while (row != NULL) {
    if (x) {
        SetType(someType);
        SetValue(someValue);
    }
    else if (y) {
        SetType(someOtherType);
        SetValue(SomeOtherValue);
    }
    // ...
    GetNextRow();
}

Edit: Another possibility would be to write your code as a for loop:

for (;row!=NULL;GetNextRow()) {
    if (!x) 
        continue;
    SetTypeAndValue();
    if (!y)
        continue;
    SetTypeandValue();
    // ...

Since the call to GetNextRow is now part of the loop itself, we don't have to (explicitly) call it each time -- the loop itself will take care of that. The next step (if you have enough of these to make it worthwhile) would be to work on shortening the code to set the types and values. One possibility would be to use template specialization:

// We never use the base template -- it just throws to indicate a problem.
template <class T>
SetValue(T const &value) { 
   throw(something);
}

// Then we provide a template specialization for each type we really use:
template <>
SetValue<int>(int value) {
    SetType(VT_INT);
    SetValue(value);
}

template <>
SetValue<float>(float value) { 
    SetType(VT_FLOAT);
    SetValue(value);
}

This lets you combine a pair of calls to set the type and the value into a single call.

Edit: As far as cutting processing short goes, it depends -- if parsing a column is expensive (enough to care about) you can simply nest your conditions:

if (x) { 
    SetTypeAndValue();
    if (y) {
        SetTypeAndValue();
        if (z) { 
            SetTypeAndValue();

and so on. The major shortcoming of this is that it'll get pretty deeply nested if (as you've said) you have 20+ conditions in a single loop. That being the case, I'd probably think hard about the for-loop based version I gave above.

Answer 6

Why not make a function to do all the work?

bool processElement(Element& element, Row* row, int value, Table& m_InferredTable, /*other params*/)
{
    if( !GetRowValue( row, m_InferredTable->DayColumn(), m_InferredTable->DayName(), m_InferredTable->TableName(), value )
            || !IsValidValue( value ) )
    {
            GetNextRow( cursor, m_InferredTable );
            return true;
    }
    else
    {
            value.ChangeType(VT_INT);
            element.SetDay( value.intVal );
    }
    return false;
}

In your loop

while (row != NULL)
{
    if (processElement(element, row, value, m_InferredTable))
        continue;
    // other code
}