Does error checking make code look ungly?

[zspirit]

I have to insert a lot of data points from a structure which looks like this:

boo result = false;

result = InsertData( temperature);

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

result = InsertData( pressure );

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

result = InsertData( location );

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

result = InsertData( .... );

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

The InsertData() function above is pusdocode but the point is that this function (or the like) returns true or false. If a false occurs anywhere it is time to abort and quite the operation. In real situation I have a lot of these InsertData() function and I need to check the value after each one. It makes the code look a little ugly and error check code/user prompt message is repetitive. Is there a way to make it better? How about I put the error checking as #define ?

#define DO_ERROR_CHECK if (result == false) \
 { \
    AfxMessageBox("An error occured"); \
    return; \
 }

// NOW...
result = InsertData( temperature);

DO_ERROR_CHECK

// and so on InsertData(....)

DO_ERROR_CHECK

Sometime I split such operation in two functions to separate and avoid repeating the message everywhere in the code.

DoOperation()
{
   result = InsertAllData();

   if (result == false)
      AfxMessageBox("An error has occured");

}

InsertAllData()
{
   if (!InsertData( temperature ) )
      return false;

   if (!InsertData( pressure ) )
      return false;
}

any thoughts? How do you do it?

[S_M_A]

Seems like you don't care what goes wrong. You could throw an exception from InsertData when something bad happens. Something like this

try
{
  InsertData( temperature );
  InsertData( pressure );
}
catch(...)
{
  AfxMessageBox( "An error occured" );
}

[Arjay]

I use exception handling and throw exceptions.

try
{
  InsertData( temperature );

  InsertData( pressure );

  ...
}
catch( CMyException& e )
{
  AfxMessageBox( ..., e.Message );
}

[GCDEF]

I agree that exception handling is probably the way to go, but you could write more concise code too.

if(!InsertData( temperature))
{
   AfxMessageBox("An error occured");
   return;
}

[Eri523]

In the particular case of your example you can even write something like this:

if (!InsertData( temperature) ||
	!InsertData( pressure ) ||
	!InsertData( location ) ||
	!InsertData( .... )
) {
   AfxMessageBox("An error occured");
   return;
}

But your real-life scenario may be more complex, not allowig that brutally simple approach.

Moreover, at least older compilers may have failed complaining about a single expression being too complex if you go too far with something like that.

There has, BTW, recently been an IMO interesting thread about that very topic over there in the Non-VC++ forum, discussing various options, even including the malicious goto...

[Igor Vartanov]

To qualify to nice or ugly, it appears very personal.
As for me this DO_ERROR_CHECK is ugly indeed:

// NOW...
result = InsertData( temperature);

DO_ERROR_CHECK

// and so on InsertData(....)

DO_ERROR_CHECK

...but I have nothing against

result = InsertData( temperature);

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

result = InsertData( pressure );

if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

I met the following approach as well:

#define CHECK(func,res) if (res) res = func; 

bool result = true;
CHECK( InsertData( temperature ), result )
CHECK( InsertData( pressure ), result )
CHECK( InsertData( location ), result )
if (result == false)
{
   AfxMessageBox("An error occured");
   return;
}

[ovidiucucu]

Although exception handling is sometimes criticized, it allows treating the errors in much more clear and centralized manner than if-else-if-else-if-...-else or call-check-return-call-check-...-return ways.

[ovidiucucu]

In the particular case of your example you can even write something like this:

if (!InsertData( temperature) ||
	!InsertData( pressure ) ||
	!InsertData( location ) ||
	!InsertData( .... )
)

I have often meet such type of "economy class" code. Luckily, I have not a gun...

[GCDEF]

I have often meet such type of "economy class" code. Luckily, I have not a gun...

I don't have a problem with that. What bothers you about it?

[zspirit]

Thank you all, I really liked all the replies. I agree exceptions are the best way to go but Igor's suggestion is quite innovative as well and other suggestions are also plausible. In my case the InsertData function is in dll which doesn't throw exceptions so I have to rely on the return value only.

To qualify to nice or ugly, it appears very personal.
As for me this DO_ERROR_CHECK is ugly indeed:

I agree and its not really that readable either but I wanted to explore.

[Arjay]

In my case the InsertData function is in dll which doesn't throw exceptions so I have to rely on the return value only.

You still can still use exceptions. Simply wrap the dll function call with a class that exposes a static method that checks the return and throws an exception.

try
{
  DllExceptionWrapper::InsertData( temperature );

  DllExceptionWrapper::InsertData( pressure );
  ...
}
catch( CMyException& e )
{
  AfxMessageBox( ..., e.Message );
}

[zspirit]

You still can still use exceptions. Simply wrap the dll function call with a class that exposes a static method that checks the return and throws an exception.

try
{
  DllExceptionWrapper::InsertData( temperature );

  DllExceptionWrapper::InsertData( pressure );
  ...
}
catch( CMyException& e )
{
  AfxMessageBox( ..., e.Message );
}

That's a great way to come around it. Say I am writing that library or dll or have access to the code, is it good idea to raise exception in the library functions? I see a trade off. Some client appilcation of the library may not want to use exception dependong on their limited use but client application risk failure if exceptions are not handled. Is wrapping them around with wrapper APIs or a class is rather better idea than raising exception in the original dll?

I am actually thinking more along side general rule now (but still making this problem as en example) and wonder if raising exception is good in a standalone library functions. Only concern I see is client application must handle those exception or risk failure and how they know which function raises which exception? MUST have documentation I guess?

[Arjay]

I generally only export C-style functions from a dll and never classes.

I also never throw exceptions on the dll level and the functions only ever return error codes.

Actually I do throw exceptions within the dll but these exceptions are caught from within and never cross the function boundary (so the consuming app doesn't need to catch any exceptions thrown by the dll).

For error passing, I usually adopt the COM error approach so each dll function returns an HRESULT.

That's what I do on the dll side of things.

On the app side of things, I like to deal with exceptions because I feel it makes the code easier to read, and error conditions are more easily cleaned up. I follow a C++ style of programming where cleanup is performed in destructors.

When I call the dll, I usually create a wrapper class that takes the dll HRESULTS and turns them into exceptions. Since the wrapper class and exceptions are defined on the app level, I get to control all of these from within the app.

[zspirit]

I generally only export C-style functions from a dll and never classes.

I also never throw exceptions on the dll level and the functions only ever return error codes.....

Thanks Arjay, your points well taken. In my case the dll exposes a class, so can't really provide a wrapper in application class but maybe I can drive from it in the dll itself and expose both classes?

For the sake of example, assume the pseudo code:

CarInDll *car;
Inventory.CreateCar( &car );
car->SetMake("BMW");
car->SetColor(BROWN);
car->SetYear(2010);
car->SetCondition("Excellent");
car->SetSalePrice(40,000);
Inventory.AddCar( &car );

I don't think I can do anything to wrap member functions of a class in application. Should I just derive another class in dll and also expose it which now is derived from the same basic export class but this one throws exceptions instead of error values like:

...in dll code now...
class CarWithException : public Car
{
void SetColor()
  {
    if (! Car::SetColor() )
      throw("requested color not available");
  }
}

[MilesAhead]

Since AfxMessageBox() returns an int you could change your function from void to int return type. Then get a single line exit by

if(! result)
  return AfxMessageBox(ErrorMsg)