The Old Joel on Software Forum: Part 5 (of 5)

Is this wrong?

In another post ( http://discuss.fogcreek.com/joelonsoftware/default.asp?cmd=show&ixPost=153178&ixReplies=34 ) this piece of code came up (or something similar).

if(thingie.isOK())
{
return true;
}
else
{
return false;
}

The question was if this code was acceptable. One possible improvement was suggested as:

return thingie.isOK();

My question is, is this ok? I remember reading once, I believe in Writing Secure Code from MS Press, that this was a bad idea. Mainly because if the call to isOK() fails, you could have a connumdrum on your hands.

If I were being really particular about the code not failing unexpectedly, I would probably write it as:

bool retVal = false; //assume not ok.
try
{
retVal = thingie.isOK();
}
catch
{
retVal = false; //or some other error response
}

return retVal;

Is this just major overkill? How would you write it differently?

Thanks for any clarification and input.

SW
Thursday, June 17, 2004

In the first two code snippets, if the IsOK() method throws an exception, the result will be the same either way. That is, the code will throw an exception at the point of invocation of IsOK(), regardless of if the calling statement is in an if block or a return statement. So for conciseness I would prefer "return thingie.IsOK();"

Whether or not exception handling is overkill or not depends entirely on what thingie.IsOK() actually does...

Joe
Thursday, June 17, 2004

Wonderful post.

Consider my humble opinion as equivalant to an idiot's rambling. I am no one to offer you anything more than an opinion. My thought process would be:

If it was a situation under C, I would design my function call around the function design.

And in this case, I would've safely used the more succinct version:

int CheckIfOk()
{
return (int)IsOk();
}

But since there's an object involved, I wouldn't risk this and would go for the try catch blocks, just in case the object reference thingie was not initialized or had already been released prior to the call.

Sathyaish Chakravarthy
Thursday, June 17, 2004

In C++ it is not possible for thingie to not be initialized. In Java I would consider it a violation of a precondition of thingie wasn't initialized and would be ok with throwing the unchecked null pointer exception.

Either way I would just do:

return thingie.isOk();

christopher baus (www.baus.net)
Thursday, June 17, 2004

Sathyaish, you turn my head inside out.

Ian
Thursday, June 17, 2004

>Sathyaish, you turn my head inside out.

Try an Asprin. :-)
Did I not say I was just offering an opinion because I was interested in the post?

Sathyaish Chakravarthy
Thursday, June 17, 2004

Shouldn't it be done like this, to avoid confusion and that cursed terse C style?

bool IsOk(ThingieObjectType oTheThingie)
{
bool fReturnVal;
bool fResultVal;

fReturnVal = false; // init fReturnVal

// get result of oTheThingie.IsOk()
fResultVal = oTheThingie.IsOk();

// if result is true then set return value to true
// otherwise set return value to false

if (fResultVal == true)
{
// oTheThingie.IsOk is true, set fReturnVal true
fReturnVal = true;
}
else
{
// oTheThingie.IsOk is false, set fReturnVal false
fReturnVal = false;
}

// now return the fReturnVal to caller

return (fReturnVal);
} // end function IsOK()

// end sarcasm

Greg Jorgensen
Thursday, June 17, 2004

and if you really don't like exceptions:

try {
return thing.isOK();
} catch (...) {
return false;
}

(btw i hate c++ having me write {} in try-catch)

A x86_64 user
Thursday, June 17, 2004

Ah, but you forgot to test for an unexpected value being returned:

if (fResultVal == true)
{
// oTheThingie.IsOk is true, set fReturnVal true
fReturnVal = true;
}
else if (fResultVal == false)
{
// oTheThingie.IsOk is false, set fReturnVal false
fReturnVal = false;
}
else
{
// oTheThingie.IsOk returned an unexpected value, raise exception
throw UnexpectedValueException;
}

Ian
Thursday, June 17, 2004

My code was so well-written and clear you had no trouble finding the error -- see how much easier it is to read and debug that style than the pointy-headed terse C style:

return thingie.isOk(); // indeed

Greg Jorgensen
Thursday, June 17, 2004

And you should write another auxilliary method, say IsThingieOKOK(), just in case your first wrapper throws an error or returns an unexpected value...and so on and so forth.

Joe
Thursday, June 17, 2004

thingie.isOk();
return true;

just ship it
Friday, June 18, 2004

Surely

{
// make some work for a new release
// thingie.isOk();

return true;
}

is by far the best solution.

Friday, June 18, 2004

If false is defined as 0 and true as non-zero, an undefined result would imply the Universe had had an Exception. I humbly suggest that Exceptions in the Universe are outside my control and if they happen all bets are off.

By which I mean, if you have a type of boolean by definition there can only be two values.

Simon Lucy
Friday, June 18, 2004

Not in (some?) databases, Simon, where tri-state boolean is the order of the day.

Friday, June 18, 2004