Software By JeffMain Page | About | Help | FAQ | Special pages | Log in
The Free Encyclopedia
Printable version | Disclaimers

Unnecessary Conditions

From Software By Jeff

The following is an exact copy of Visual C++ code pulled from a real project. Only the class name (CMyDlg) has been changed to protect the innocent; the client didn't write the code, but they have this kind of bad code in their source:

void CMyDlg::OnFileStartJob()
	if ( !StartJob(NULL) )  //Added if and return VJH V1.3 

If you're the VJH who wrote this, shame on you.

"What's wrong with this?" you may ask. Obviously it's valid C++ code; it compiles and runs.

First, we can assume from this bit of code that StartJob() is returning something we're treating as boolean. We call StartJob() and validate its return. Fair enough.

When it returns, all we do is also return. So if StartJob() returns false or some value that evaluates to zero we return because of the return statement. If StartJob() returns true or some non-zero value we skip the return statement, and then return because we're done anyway.

So, why bother with the if()? There is no good reason. Really. There is no good reason.

Let's look at what the code should be:

void CMyDlg::OnFileStartJob()

Woah! Much better. Yeah, the comment could have remained, but really, since it attempted to explain the if(), there's no reason to keep it.

"Why bother even having OnFileStartJob() call StartJob()?" you may ask. Well, the NULL parameter for one; it may (nay, better) be the case that StartJob() is called somewhere else with a different parameter list. In that case, having our method (which looks like, and actually is, the action taken because of a menu selection or button activiation) wrap the call is perfectly valid. We assume, then, that the other calls fill the parameter list with something appropriate for that call.

If, however, this is the only place that StartJob() is called, then the entirety of StartJob() should be in OnFileStartJob(). There are asthetically valid reasons to call the one from the other, but no technically good reasons (that is, no reason to call a method just because you can). Perhaps StartJob() is really long and convoluted and therefore kept in its own .cpp file Fair enough. If this is the case, however, loose the parameter list.

Remember: never write more than is necessary. Someone will curse your name for the Internet to see...

Retrieved from ""

This page has been accessed 8606 times. This page was last modified 00:00, 9 Dec 2007.

Main Page
Community portal
Current events
Recent changes
Random page
Edit this page
Editing help
This page
Discuss this page
Post a comment
Printable version
Page history
What links here
Related changes
My pages
Create an account or log in
Special pages
New pages
Image list
Bug reports