Throwing exceptions to control flow - code smell?
Is it really important for doC() to be executed when doB() fails? If not, why not simply let the Exception propagate up the stack to where it can be handled effectively. Personally, I consider using error codes a code smell.
Edit: In your comment, you have described exactly the scenarion where you should simply declare
public void doSomething() throws MyException
It entirely depends on what that error condition is, and what the method's job is. If returning ERROR
is a valid way of handling that error for the calling function, why would it be bad?
Often, however, it is a smell. Consider this:
bool isDouble(string someString) {
try {
double d = Convert.ParseInt32(someString);
} catch(FormatException e) {
return false;
}
return true;
}
That is a very big code smell, because you don't expect a double value. You just want to know whether a string contains a double.
Sometimes, the framework you use doesn't have other ways of doing what you want. For the above, there is a better way:
bool isDouble(string someString) {
bool success;
Convert.TryParseInt32(someString, ref success);
return success;
}
Those kinds of exceptions have a special name, coined by someone whose blog I read recently, but sadly, I forgot its name. Please comment if you know it. Last but not least, the above is pseudocode. I'm not a C# developer so the above doesn't compile, I'm sure, but TryParseInt32
/ ParseInt32
demonstrates that well I think, so I'm going with C#.
Now, to your code. Let's inspect two functions. One smells, and the other doesn't:
1. Smell
public int setupSystem() {
doA();
try { doB(); }
catch (MyException e)
{ return ERROR; }
doC();
return SUCCESS;
}
That's a code smell, because when you want to setup a system, you don't want it to fail. Failing to setup a system means you can't continue without handling that error.
2. OK
public int pingWorkstation() {
doA();
try { doB(); }
catch (MyException e)
{ return ERROR; }
doC();
return SUCCESS;
}
That is OK, because the purpose of that method is to test whether the workstation is still reachable. If it's not, then that is part of the result of that method, and not an exceptional case that needs an alternative return path.
My only problem with the OP's code is that you're mixing paradigms -- doB shows an error by throwing an exception, while doSomething shows an error by returning a code. Ideally, you would pick one or the other. Of course, in legacy maintenance, you might not have that luxury.
If you pick returning error codes, that's OK, but I dislike it because it encourages you to use side channels (like global variables) to communicate state on failure, rather than bundling that state into an exception and letting it bubble up the stack until you can do something about it.