IMO, the previous answers focus on edge cases and code which should be rewritten anyway. Here are some cases I see nearly everywhere in which if/else should have been replaced by if or another language construct.
Note: I make the answer community wiki. Feel free to modify it and add other examples of cases where if/else usually leads to bad code.
Beginners, ye be warned! if/else has a potential of creating an unreadable code when not used carefully. The next examples show the most common cases where if/else is easily misused.
1. Guard clause
I stopped counting how many times I've seen something like this:
void Demo(...)
{
if (ok_to_continue)
{
... // 20 lines here.
}
else
{
throw new SomeException();
}
}
Nobody needs extra indentation. The same code could have been written like this:
void Demo(...)
{
if (!ok_to_continue)
{
throw new SomeException();
}
... // No indentation for the next 20 lines.
}
Rule: if you can remove an else statement by inverting the if check, handling an error, and exiting immediately, (else otherwise continue on), then do so.
2. Code duplication
This one is also quite frequent.
if (!this.useAlternatePanel)
{
currentInput = this.defaultTextBox.Text;
bool missingInput = currentInput.Length == 0;
this.defaultProcessButton.Enabled = !missingInput;
this.defaultMissingInputHelpLabel.Visible = missingInput;
}
else
{
currentInput = this.alternateTextBox.Text;
bool missingInput = currentInput.Length == 0;
this.alternateProcessButton.Enabled = !missingInput;
this.alternateMissingInputHelpLabel.Visible = missingInput;
}
This is an excellent example of beginners' code, because a beginner wouldn't always see how such code can be refactored. One of the ways is to create a map of controls:
var defaultControls = new CountriesProcessingControls(
this.defaultTextBox,
this.defaultProcessButton,
this.defaultMissingInputHelpLabel);
var alternateControls = new CountriesProcessingControls(
this.alternateTextBox,
this.alternateProcessButton,
this.alternateMissingInputHelpLabel);
void RefreshCountriesControls(CountriesProcessingControls controls)
{
currentInput = controls.textBox.Text;
bool missingInput = currentInput.Length == 0;
controls.processButton.Enabled = !missingInput;
controls.missingInputHelpLabel.Visible = missingInput;
}
RefreshCountriesControls(this.useAlternatePanel ? alternateControls : defaultControls);
Rule: if the block in else is nearly identical to the block in if, you're doing it wrong and causing code duplication.
3. Trying to do two things
The opposite is also common:
if (...)
{
foreach (var dateInput = this.selectedDates)
{
var isConvertible = convertToDate(dateInput);
if (!isConvertible)
{
this.invalidDates.add(dateInput);
}
}
}
else
{
var languageService = this.initializeLanguageService();
this.Translated = languageService.translate(this.LastInput);
if (this.Translated != null)
{
this.NotificationScheduler.append(LAST_INPUT_TRANSLATED);
}
}
The block within if has nothing to do with the block within else. Combining two blocks in the same method makes the logic difficult to understand and error-prone. Finding a name for the method, as well as commenting it properly is extremely difficult too.
Rule: Avoid combining blocks which have nothing in common together in an if/else block.
4. Object-oriented programming
This sort of coding is also frequent among beginners:
// `animal` is either a dog or a cat.
if (animal.IsDog)
{
animal.EatPedigree();
animal.Bark();
}
else // The animal is a cat.
{
animal.EatWhiskas();
animal.Meow();
}
In object-oriented programming, this code shouldn't exist. It should be written like this instead:
IAnimal animal = ...;
animal.Eat();
animal.MakeSound();
given that each class (Cat : IAnimal and Dog : IAnimal) will have their own implementation of those methods.
Rule: when doing object-oriented programming, generalize functions (methods) and use inheritance and polymorphism.
5. Assigning a value
I see this one everywhere in codebases written by beginners:
int price;
if (this.isDiscount)
{
price = -discount;
}
else
{
price = unitCost * quantity;
}
This is a terrible piece of code, since it's not written for humans.
It is misleading. It makes you think that the main operation here is the choosing between discount and non-discount mode, while the actual main operation is the assignment of price.
It is error prone. What if later, one condition is changed? What if one of two assignments is removed?
It invites even worse code. I've seen a lot of pieces like that where int price was instead int price = 0;, just to get rid of compiler warning if one of the assignments is missing. Hiding warnings instead of solving them is one of the worst things one can do.
There is a tiny code duplication, which means more characters to type originally, and more changes to do later.
It's uselessly long.
The same could be written like this:
int price = this.isDiscount ? -discount : unitCost * quantity;
Rule: use the ternary operator instead of if/else blocks for simple value-assignments.
