stevedoria.net

Scratching your head doesn't make it work any better. It only loosens unsightly dandruff.

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. if variable_A is non-zero
   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:

1. connect to database_server_A using host_A, login_A, password_A
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

Leave a Reply