Consider this bit of code:
if (x == 1)
{
throw "no good; aborting" ;
}
[... more code ...]
Now consider this code:
if (x == 1)
{
throw "no good; aborting" ;
}
else
{
[... more code ...]
}
The two cases work exactly the same way. The first case has the advantage that you don’t have to “encase” the rest of the code in an else. The second has the advantage of following the practice of explicitly having an else for every if.
Can anyone provide any solid arguments in favor of one over the other?
6
You should not add else after if branches that break control flow unconditionally, such as ones containing a throw or a return. This would improve readability of your program by removing the unnecessary level of nesting introduced by the else branch.
What looks more or less OK with a single throw becomes truly ugly when several throws in a row get involded:
void myMethod(int arg1, int arg2, int arg3) {
// This is demonstrably ugly - do not code like that!
if (!isValid(arg1)) {
throw new ArgumentException("arg1 is invalid");
} else {
if (!isValid(arg2)) {
throw new ArgumentException("arg2 is invalid");
} else {
if (!isValid(arg3)) {
throw new ArgumentException("arg3 is invalid");
} else {
// The useful code starts here
}
}
}
}
This snippet does the same thing, but it looks much better:
void myMethod(int arg1, int arg2, int arg3) {
if (!isValid(arg1)) {
throw new ArgumentException("arg1 is invalid");
}
if (!isValid(arg2)) {
throw new ArgumentException("arg2 is invalid");
}
if (!isValid(arg3)) {
throw new ArgumentException("arg3 is invalid");
}
// The useful code starts here
}
5
I would call the “explicit else” practice you refer to as an anti-pattern, as it obscures the fact that there is no special-case code as an else to your if.
Readability/maintainability is generally improved when you mostly have nothing but necessary code-flow constructs, and you minimize them. This means redundant elses and if’s which will add a scope to an entire function make following and maintaining it more difficult.
Say for example you have this function:
public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
if (_validColors.Contains(oblogonToConfigure.Color))
{
oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
}
else
{
oblogonToConfigure.Color = _validColors[0];
oblogonToConfigure.ColorIndex = 0;
}
}
Now the requirement comes in that during configuration you should also specify the oblogon’s type/type index, there are multiple scopes which someone could place that code and end up with invalid code i.e.
public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
if (!_validOblogons.Contains(oblogonToConfigure.Type))
{
oblogonToConfigure.Type = _validOblogons[0];
oblogonToConfigure.TypeIndex = 0;
if (_validColors.Contains(oblogonToConfigure.Color))
{
oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
}
else
{
oblogonToConfigure.Color = _validColors[0];
oblogonToConfigure.ColorIndex = 0;
}
}
else
{
oblogonToConfigure.TypeIndex = _validOblogons.IndexOf(oblogonToConfigure.Type);
}
}
Compare this to if the original code were written with minimal control flow constructs necessary and minimized ones at that.
public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
if (!_validColors.Contains(oblogonToConfigure.Color))
{
oblogonToConfigure.Color = _validColors[0];
}
oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
}
It would now be far more difficult to accidentally put something in the wrong scope or end up bloating scopes causing duplication in the long term growth and maintenance of this function. Plus it’s obvious what the possible flows through this function are so readability is enhanced.
I know, the example’s a bit contrived, but I have many times seen
SomeFunction()
{
if (isvalid)
{
/* ENTIRE FUNCTION */
}
/* Nothing should go here but something does on accident, and an invalid scenario is created. */
}
So formalizing those rules about control-flow constructs I think may help folks develop the intuition necessary to smell something when they start writing code like that. Then they will start to write..
SomeFunction()
{
if (!isvalid)
{
/* Nothing should go here, and it's so small no one will likely accidentally put something here */
return;
}
/* ENTIRE FUNCTION */
}