Redundant code constructs

Question

The most egregiously redundant code construct I often see involves using the code sequence

if (condition)
    return true;
else
    return false;

instead of simply writing

return (condition);

I've seen this beginner error in all sorts of languages: from Pascal and C to PHP and Java. What other such constructs would you flag in a code review?

Answer 1

Returning uselessly at the end:

   // stuff
   return;
}

Answer 2

Using .tostring on a string

Answer 3

if (condition == true)
{
  ...
}

instead of

if (condition)
{
  ...
}

Edit:

or even worse and turning around the conditional test:

if (condition == false)
{
  ...
}

which is easily read as

if (condition) then ...

Answer 4

if (foo == true)
{
   do stuff
}

I keep telling the developer that does that that it should be

if ((foo == true) == true)
{
   do stuff
}

but he hasn't gotten the hint yet.

Answer 5

void myfunction() {
  if(condition) {
    // Do some stuff
    if(othercond) {
      // Do more stuff
    }
  }
}

instead of

void myfunction() {
  if(!condition)
    return;

  // Do some stuff

  if(!othercond)
    return;

  // Do more stuff
}

Answer 6

I once had a guy who repeatedly did this:

bool a;
bool b;
...
if (a == true)
    b = true;
else
    b = false;

Answer 7

Putting an exit statement as first statement in a function to disable the execution of that function, instead of one of the following options:

• Completely removing the function
• Commenting the function body
• Keeping the function but deleting all the code

Using the exit as first statement makes it very hard to spot, you can easily read over it.

Answer 8

Declaring separately from assignment in languages other than C:
int foo;
foo = GetFoo();

Answer 9

Using comments instead of source control:
-Commenting out or renaming functions instead of deleting them and trusting that source control can get them back for you if needed.
-Adding comments like "RWF Change" instead of just making the change and letting source control assign the blame.

Answer 10

Somewhere I’ve spotted this thing, which I find to be the pinnacle of boolean redundancy:

return (test == 1)? ((test == 0) ? 0 : 1) : ((test == 0) ? 0 : 1);

:-)

Answer 11

I often run into the following:

function foo() {
    if ( something ) {
        return;
    } else {
        do_something();
    }
}

But it doesn't help telling them that the else is useless here. It has to be either

function foo() {
    if ( something ) {
        return;
    }
    do_something();
}

or - depending on the length of checks that are done before do_something():

function foo() {
    if ( !something ) {
        do_something();
    }
}

Answer 12

Using an array when you want set behavior. You need to check everything to make sure its not in the array before you insert it, which makes your code longer and slower.

Answer 13

Redundant code is not in itself an error. But if you're really trying to save every character

return (condition);

is redundant too. You can write:

return condition;

Answer 14

Fear of null (this also can lead to serious problems):

if (name != null)
  person.Name = name;

Redundant if's (not using else):

if (!IsPostback)
{
   // do something
}
if (IsPostback)
{
   // do something else
}

Redundant checks (Split never returns null):

string[] words = sentence.Split(' ');
if (words != null)

More on checks (the second check is redundant if you are going to loop)

if (myArray != null && myArray.Length > 0)
  foreach (string s in myArray)

And my favorite for ASP.NET: Scattered DataBinds all over the code in order to make the page render.

Answer 15

Copy paste redundancy:

if (x > 0)
{
   // a lot of code to calculate z
   y = x + z;
}
else
{
   // a lot of code to calculate z
   y = x - z;
}

instead of

if (x > 0)
  y = x + CalcZ(x);
else
  y = x - CalcZ(x);

or even better (or more obfuscated)

y = x + (x > 0 ? 1 : -1) * CalcZ(x)

Answer 16

Allocating elements on the heap instead of the stack.

{
    char buff = malloc(1024);
    /* ... */
    free(buff);
}

instead of

{
    char buff[1024];
    /* ... */
}

or

{    
    struct foo *x = (struct foo *)malloc(sizeof(struct foo));
    x->a = ...;
    bar(x);
    free(x);
}

instead of

{
    struct foo x;
    x.a = ...;
    bar(&x);
}

Answer 17

From nightmarish code reviews.....

char s[100];

followed by

memset(s,0,100);

followed by

s[strlen(s)] = 0;

with lots of nasty

if (strcmp(s, "1") == 0)

littered about the code.

Answer 18

Redundant .ToString() invocations:

const int foo = 5;
Console.WriteLine("Number of Items: " + foo.ToString());

Unnecessary string formatting:

const int foo = 5;
Console.WriteLine("Number of Items: {0}", foo);

Answer 19

The most common redundant code construct I see is code that is never called from anywhere in the program.

The other is design patterns used where there is no point in using them. For example, writing "new BobFactory().createBob()" everywhere, instead of just writing "new Bob()".

Deleting unused and unnecessary code can massively improve the quality of the system and the team's ability to maintain it. The benefits are often startling to teams who have never considered deleting unnecessary code from their system. I once performed a code review by sitting with a team and deleting over half the code in their project without changing the functionality of their system. I thought they'd be offended but they frequently asked me back for design advice and feedback after that.