C# - If Statement problem?

Question

Not sure exactly what is wrong here. I'm not sure if I am supposed to be using "else if" or what. Here's the code:

    private void txtMessage_TextChanged(object sender, EventArgs e)
    {
        int length = txtMessage.TextLength;
        int left = 140 - length;
        charactersleft.Text = left.ToString() +  " characters left";

        if (left < 140)
        {
            charactersleft.ForeColor = Color.Green;
        }

        if (left < 110)
        {
            charactersleft.ForeColor = Color.Yellow;
        }

        if (left < 80)
        {
            charactersleft.ForeColor = Color.Orange;
        }

        if (left < 50)
        {
            charactersleft.ForeColor = Color.Red;
        }

        else
        {
            charactersleft.ForeColor = Color.Black;
        }
    }

The forecolor of the label "charactersleft" is supposed to change depending on the number of characters within txtMessage. But for some reason it's not working. I'm sure the solution is simple, I'm just easily confused with the whole "else if" thing. Thanks in advance.

Answer 1

The problem here is if the length is 30, it will match every if. You should reverse the order and use else if for each statement:

if (< 50)      // red
else if (< 80) // orange
...
else           //black

Answer 2

Change them to:

    if (left < 50)
    {
        charactersleft.ForeColor = Color.Green;
    }
    else if (left < 80)
    {
        charactersleft.ForeColor = Color.Yellow;
    }
    else if (left < 110)
    {
        charactersleft.ForeColor = Color.Orange;
    }
    else if (left < 140)
    {
        charactersleft.ForeColor = Color.Red;
    }
    else
    {
        charactersleft.ForeColor = Color.Black;
    }

Answer 3

Yes, you need to do that in elseif's, because as it is right now, every condition is a separate statement, which means that the code will execute all the way down to if(left < 50) and then it will be either red, or black, as the else-clause will match everything that's > 50.

Answer 4

The problem is that each of your if statements is being treated as a separate statement, and so your else statement is being executed whenever the last if statement is false, i.e. whenever left >= 50

If you use else if in place of if then this becomes 1 if statement, and the final else will only be executed if all of the previous statements were false.

You should also reverse the order of your if statements to check the "least likely" case first:

private void txtMessage_TextChanged(object sender, EventArgs e)
{
    int length = txtMessage.TextLength;
    int left = 140 - length;
    charactersleft.Text = left.ToString() +  " characters left";

    if (left < 50)
    {
        charactersleft.ForeColor = Color.Red;
    }
    else if (left < 80)
    {
        charactersleft.ForeColor = Color.Orange;
    }
    else if (left < 110)
    {
        charactersleft.ForeColor = Color.Yellow;
    }
    else if (left < 140)
    {
        charactersleft.ForeColor = Color.Green;
    }
    else
    {
        charactersleft.ForeColor = Color.Black;
    }
}

Answer 5

Consider using else if, even though it looks uglier. All of the conditions are true if the first one is true, so it'll always go to Black.

Answer 6

It's the last If condition; unless left < 50, you will always revert the label's forecolor to black.

Answer 7

I would define a list of Colors in the proper order :

private List<Color> clrList = new List<Color> 
{
    Color.Black,
    Color.Orange,
    Color.Yellow,
    Color.Red,
    Color.Green
};

And then write a function that given a length input returned the right color.

It might look like this :

private Color selectColor(int strLength)
{
    int ndx;

    strLength = 140 - strLength;

    if (strLength < 1)
    {
       // what's the default ndx color to return for strings of length > 140 ?

       if(strLength < 1) ndx = ?? // left for you to complete

    }
    else
    {
       // take advantage of the pattern of increments of #30 to calculate the color index
       ndx = ?? // left for you to complete
    }

    // so now use 'ndx to pull the correct color out of 'clrList
    return clrList[ndx];
}

In this case you can see a "pattern" in the data "criteria" : you go from 50 to 140 by a step value of 30, and it looks like there's a "default case" you can handle where the message string is so big that (140 - messageString.Length) is going to be < 0.

I'd also pull the value #140 out of the color select function, and make it a variable or property, or a parameter passed into the color select functin, and try and re-factor the code so that it had, possibly, many uses in scenarios where the there was a regular pattern in the increment used to change which data is accessed.