Code Critique: Duplicate Code within If-Else Statements
I’ve been reviewing and documenting other peoples’ code lately, and I found some vexing patterns that came up constantly. Seeing in an if-else statement that the first (or last) few lines evaluated when the conditional is true match the first (or last) few lines evaluated when the conditional is false was a little bothersome. It is as if someone copied code in their editor and pasted it where they needed it.
An example snippet of code is:
function f()
{
if( g() )
{
// statement_1
// statement_2
// ...
// statement_n
// some stuff
}
else
{
// statement_1
// statement_2
// ...
// statement_n
// other stuff
}
}
Firstly, if a sequence of code is present in two or more places, wrapping the sequence within a function and calling the function may make the code just a bit more elegant. Wrapping identical sequences of code within a function localizes the code that needs to be updated when the sequence of code needs to be changed. Localizing common sequences of code within a function eliminates the need for programmers to track down all identical code sequences where and when an update is desired.
Wrapping statement_1 through statement_n in a function, h(), results in the following code:
function h()
{
// statement_1
// statement_2
// ...
// statement_n
}
function f()
{
if( g() )
{
h();
// some stuff
}
else
{
h();
// other stuff
}
}
The code’s annoyance factor can be further minimized by noting that h() is the first statement that is evaluated after the if-else conditional expression evaluates to either true or false. Since h() is evaluated without dependence on the if-else conditional expression, it seems sensible to move h() out of the if-else statement.
The resulting code is presented here:
function h()
{
// statement_1
// statement_2
// ...
// statement_n
}
function f()
{
h();
if( g() )
{
// some stuff
}
else
{
// other stuff
}
}
I noticed the issue after writing pseudocode that closely matched existing code and looked like this:
1a – i. connect to database_server_A using host_A, login_A, password_A
1a – ii. use schema_A as the default schema
1a – iii. do stuff
1b. else
1b – i. connect to database_server_A using host_A, login_A, password_A
1b – ii. use schema_A as the default schema
1b – iii. do alternate stuff
I thought that the following pseudocode would be easier to read and more concise:
2. use schema_A as the default schema
3a. if variable_A is non-zero
3a – i. do stuff
3b. else
3b – i. do alternate stuff