FuglyCode of the Week

• Error Handling vs Method Overloading
• Multiple Nested If Statements

This week’s fugly code comes from my past experiences as a C developer, and is actually one of the first pieces of advice my Team Lead gave me when I joined his group.

You see, I was working on this function that returned a value based on a few incoming parameters, and the lead admitted he was having difficulty reading the code.  It looked something like the following snippet:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

int myFunction( int a, int b, int c )
{
  /* Initialize return value */
  int returnValue = 0;
 
  /* Determine return value */
  if ( a == 0 || b == 0 || c == 0 )
  {
    /* Provided values are invalid, return -1 */
    returnValue = -1;
  }
  else
  {
    if ( a > b )
    {
      returnValue = 1;
    }
    else if ( b > c )
    {
      returnValue = 2;
    }
    else if ( c > a )
    {
      returnValue = 3;
    }
    else
    {
      returnValue = 0;
    }
 
    // Do more stuff here
 
  }
 
  /* Return the value */
  return( returnValue );
}

Lets take a minute to review this function…

• At line 1, we begin our function and accept 3 integers
• At line 4, we create a variable to hold the value our function will return upon completion
• At line 7, we enter an if statement and check to see if any of the provided values are 0
• If any of the values are 0, then we set our return value to -1
• Otherwise, we enter our else block and determine our return value
• At line 31, we have code that does other important stuff
• Finally, at line 36, we return our return value

Note: the “other stuff” code at line 31 was not reproduced as we don’t really care what it does in this example, but we do need to know that it is there.

My Lead’s beef with the function above mostly had to do with lines 7 through 33. Specifically, he deplored the fact that the if statement was the length of the function, and also questioned the reasoning behind letting the function continue its flow if the parameters were determined to be incorrect. (For example, if we add code to line 34, it will be run regardless of the parameters being valid or not.)

He explained that, although nested if statements could be useful from time to time, they should generally be avoided as they make code difficult to maintain.

So I asked him how I could avoid nested if statements, and he proposed that I simply return the -1 value at line 10, instead of assigning it to a variable and returning it later on. This would simplify the function, and would remove the function-long if statement:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35

int myFunction( int a, int b, int c )
{
  /* Initialize return value */
  int returnValue = 0;
 
  /* Validate Parameters */
  if ( a == 0 || b == 0 || c == 0 )
  {
    /* Provided values are invalid, return -1 */
    return( -1 );
  }
 
  /* Determine return value */
  if ( a > b )
  {
    returnValue = 1;
  }
  else if ( b > c )
  {
    returnValue = 2;
  }
  else if ( c > a )
  {
    returnValue = 3;
  }
  else
  {
    returnValue = 0;
  }
 
  // Do more stuff here
 
  /* Return the value */
  return( returnValue );
}

And there we go. By immediately returning the value -1, we can remove the else block and avoid putting the whole function in an if statement.

This might not seem like much right now, but one day you might run into a very large function with multiple lines of code and multiple if statements that persist throughout the function. And on that day you’ll hit your forehead with your hand and wonder why the function’s author didn’t read FuglyCode.com

Function Optimization

This function could be even more optimized. Can you figure out how?

Try rewriting this function to make it as optimal as possible and then compare what you wrote with my code (click on the down arrow to expand the code box). Feel free to share your code in the comments if its any different from mine!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34

int myFunction( int a, int b, int c )
{
  /* Validate Parameters */
  if ( a == 0 || b == 0 || c == 0 )
  {
    /* Provided values are invalid, return -1 */
    return( -1 );
  }
 
  /* Call new doMoreStuff function */
  doMoreStuff();
 
  /* Return a value */
  if ( a > b )
  {
    return( 1 );
  }
  else if ( b > c )
  {
    return( 2 );
  }
  else if ( c > a )
  {
    return( 3 );
  }
 
  return( 0 );
}
 
void doMoreStuff ()
{
  /* Move the do more stuff into its own function *
   * to increase reusability and readability */
}