current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite

This example comes from a code snippet posted on Stack Overflow:

private double brickVal(Color c) {
    if (c == Color.RED) {
        return 10.0;
    } else if (c == Color.ORANGE) {
        return 8.0;
    } else if (c == Color.YELLOW) {
        return 6.0;
    } else if (c == Color.GREEN) {
        return 4.0;
    } else if (c == Color.CYAN) {
        return 2.0;
    } else if (c == Color.MAGENTA) {
        return 1.0;
    } else {
        return 1.0;
    }
}

Is the use of == here correct / best practice?

share|improve this question
1  
I wouldn't use both in the same function! In what context is this function being used? –  Eric Apr 23 '11 at 11:11
add comment

4 Answers

up vote 12 down vote accepted

No, it's not correct (and thus of course also not best practice). For example the condition c == Color.GREEN will be true if the method is called as brickVal(Color.GREEN), but false if it is called as brickVal(new Color(0, 255, 0)). Since Color.GREEN is equal to new Color(0, 255, 0) this behavior is most likely unintentional (at least I can't imagine a scenario where you'd want for brickVal(Color.GREEN) to behave differently than brickVal(new Color(0,255,0)).

Of course if you know that the method will only ever be called using the "pre-made" colors and never using new Color, it will behave correctly. However I'd still advise against using ==. I can see no good reason to not use equals and using == comes with the danger that someone might call brickVal with new Color anyway, not knowing that they're not supposed to.

Further given the fact that brickVal apparently is meant to be only called with some specific colors as arguments and it doesn't use any properties of the colors other than their identity, I think an enum might be more suitable here than using the Color class. This way you can use switch instead of if ... else if ... and don't have to worry about anybody passing in new Color as the argument.

As a somewhat unrelated note, I find it confusing that the brickVal variable is set in every case except if the argument is RED and in the else case. Obviously I don't know how and where the brickVal variable is going to be used, but this seems like a design smell to me. I also think it's a bad idea to have a variable with the same name as the method.

share|improve this answer
    
EDIT-ed question to get rid of the brickval = ... and other cruft that detracted from my main question. –  Stephen C Apr 23 '11 at 14:32
add comment
up vote 5 down vote

My take on this is that it is bad practice.

In some circumstances, this code could be "correct" ... in the sense of being bug free as written. Specifically, if the entire application was written to use only a predefined palette of Color objects ... and there was no way that it could introduce new Color objects into this calculation.

The problem is that this is potentially a global (application wide) invariant. Any change to any class might potentially introduce new Color objects and violate the invariant. In short the application is at best "fragile".

The other problem is that this dependency on an global invariant makes the class hard to reuse.

For these reasons, my take is that this approach is bad practice ... whether or not it is buggy in the context of the application.

share|improve this answer
1  
I dont get it. The only problem I see is == vs 'equals', and maybe he should use a lookuptable instead of the repeated ifs. –  Martin Wickman Apr 24 '11 at 13:09
    
@Martin - == versus equals is the point. –  Stephen C Apr 26 '11 at 5:53
add comment
up vote 3 down vote

Not a good practice.

In the manner that is seems you want to use color - the thing that matters is the value. You want to know if the actual color is the same. It doesn't matter if it is 2 different objects that have the same values or the same object. And == checks if it is the same object. what you want to use is equals() because equals() checks if the two colors have the same values.

share|improve this answer
add comment
up vote 4 down vote

The argument c is an instance of Color. When you use the == operator you are checking if the instances are pointing to the same object. In this case (and most cases that I encounter), you do not want to compare pointers, you want to compare values. After all, Color is a class that encapsulates a color value. Here is an example:

import java.awt.Color;

class ColorTest {

    public static void main(String args[]) {
        Color c1 = Color.RED;
        Color c2 = new Color(255,0,0);
        System.out.println(c1 == c2); //false - not pointing to the same object
        System.out.println(c1.equals(c2)); //true - both are red
    }
}

This is why all java objects have a equals() method, which is inherited from the Object class: we don't care where it is physically stored in memory, we care about what it represents.

EDIT: So to answer the question "Is this good practice" I would say no it is not.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.