2

A simplified example:

function logTheColor (color){
    if(color == "red"){
        color = "The color is red "
    } else if (color == "yellow") {
        color = "The color is yellow "
    } else {
        color =  "The color is blue"
    }   
    console.log(color);
}

Is there something wrong with overwriting the value of the variable inside an if statement, if that variable is also part of the if statement condition? My question is not about the ways I can achieve the same in a "clean" way, I just want to know if it's right although it seems like a bad practice.
I don't see any problem in this particular case cause I don't have to re-evaluate the "color" variable with the original content again. Am I missing something? Are there cases when it really matters?

Alejandro Veltri
  • 131
  • 1
  • 6

4 Answers4

3

Who 'owns' the parameter passed in?

Consider the situation where the value of color gets modified by the function and its passed in by reference. This may not always be something that you can control (pass by value vs pass by reference).

#include <iostream>
using namespace std;

void doFoo(int& arg) {
    arg++;
}

int main(void) {
    int foo = 42;
    doFoo(foo);
    cout << foo;
    return 0;
}

https://ideone.com/RioRXb

This code prints out 43. The function doFoo got the argument by reference and modified it. This may be what its supposed to do, and there are certainly times to be aware of it. But, if the function doesn't 'own' the argument when it comes in, such a side effect can be quite bad.

This concept of ownership is a real one and has implications in places. In pre-arc Objective C, one would actually call retain and release on things if you wanted to 'own' them. The alternative, you could make a copy of it (not what you're addressing) and own that copy.

Often, the best practice is to consider that the arguments passed in are immutable final things that you shouldn't touch. Reassigning parameters is often seen as a questionable practice - even if the thing can't change. The specifics of if it can change from language to language, or even silently within a language (the perceived difference in passing an Object and an int in Java).

Changing a thing

Setting aside the parameter being modified, code like this sets up the possibility/likelihood of becoming unmaintainable over time.

Modifying the condition for the if (or index in a for loop), in a series of if statements (yes, you've got an else in there) can lead to code where you depend on a value changing state in a previous if, that doesn't (and it has a different state).

This starts getting into rather contrived examples trying to demonstrate it in a short bit of code. Often this comes about with larger blocks of code where you think that nothing later down uses it, so its safe to modify... and then someone comes in, looks at the assignments, looks at the hundred lines of code (you'll find this in code you maintain that "someone else wrote" all the time), skim to where you need to make the change... make the change, and find out that the thing you have here now, isn't the thing that was at the top of the block.

The example code just... its got a whiff of a code smell that has the scent of Same Name Different Meaning - the identical identifiers which would be addressed by Split Temporary Variable. That this isn't a temporary variable is also problematic.

Community
  • 1
1

I will not say it's always wrong but it's generally a bad idea.

  • Function names shouldn't lie.
  • Function name says it logs the color.
  • Function name doesn't say it changes the color.
  • Function name lies.
  • Function changes color as a side effect.
  • Side effects in functions are not a good idea according to Uncle Bob's Clean Code
  • If you do that in many places in your code, the code will be a maintenance nightmare.

Please don't code as if you will throw the code away immediately. Code as if your code will be re-used in the future, not only by you but by others.

Tulains Córdova
  • 39,201
  • 12
  • 97
  • 154
1

For this particular example, for this particular language, it happens to be technically correct. However, consider this quote from Tony Hoare:

There are two ways to write code: write code so simple there are obviously no bugs in it, or write code so complex that there are no obvious bugs in it.

The problem with your example is you can't look at it and say there are obviously no bugs in it. I don't write JavaScript code every day, but I've written a few apps that comprise several thousand lines of code each. I had to write and run a test to determine your example didn't have bugs.

If color were passed by reference, you would have created a bug.

If you were working in a language where a new scope was created inside each conditional block, thereby creating a new variable also named color that went out of scope immediately after being assigned, you would have created a bug.

If a future change ever needs to later use the original color value, you've forced a refactor.

Every single time someone reads your function in the future, they will be expending mental effort keeping the two different colors straight, which makes it that much harder to understand and maintain the code.

Don't make your coding standard the minimum you can get away with and your program works. Make it so simple there are obviously no bugs in it.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
0

Here you are recycling a variable in order to not need declare a local variable. The name of the original variable color becomes some notion like color_description. It is bad style to recycle variables, especially for a different entity.

It really is no effort to write:

var description;
if (color == "red") {
    description = "The color is red ";
} else if (color == "yellow") {
    description = "The color is yellow ";
} else {
    description = "The color is blue";
}

Or

var description =
    color == "red" ? "The color is red "
    : color == "yellow" ? "The color is yellow "
    : "The color is blue";
Joop Eggen
  • 2,011
  • 12
  • 10