7
votes
\$\begingroup\$

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?

\$\endgroup\$

closed as off-topic by nhgrif, Vogel612, Jeroen Vannevel, Heslacher, Pimgd Aug 4 '14 at 11:33

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – nhgrif, Vogel612, Jeroen Vannevel, Heslacher, Pimgd
If this question can be reworded to fit the rules in the help center, please edit the question.

locked by Jamal Dec 21 '14 at 21:56

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site, so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. More info: help center.

Read more about locked posts here.

  • 1
    \$\begingroup\$ I wouldn't use both in the same function! In what context is this function being used? \$\endgroup\$ – Eric Apr 23 '11 at 11:11
  • \$\begingroup\$ This question was mentioned on meta. \$\endgroup\$ – Pimgd Aug 5 '14 at 7:57
13
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ EDIT-ed question to get rid of the brickval = ... and other cruft that detracted from my main question. \$\endgroup\$ – Stephen C Apr 23 '11 at 14:32
5
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I dont get it. The only problem I see is == vs 'equals', and maybe he should use a lookuptable instead of the repeated ifs. \$\endgroup\$ – Martin Wickman Apr 24 '11 at 13:09
  • \$\begingroup\$ @Martin - == versus equals is the point. \$\endgroup\$ – Stephen C Apr 26 '11 at 5:53
4
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ We actually do care a little about their memory location. The first line of any good equals() function will always be if (a == b) return true; \$\endgroup\$ – nhgrif Aug 4 '14 at 11:26
3
votes
\$\begingroup\$

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.

\$\endgroup\$
0
votes
\$\begingroup\$

I tried this and it worked very well:

Color c1 = Color.WHITE;
Color c2 = new Color(255,255,255);

if(c1.getRGB() == c2.getRGB()) 

    System.out.println("true");

else

    System.out.println("false");

}

getRGB function returns an int value with the sum of Red Blue and Green, so we are comparing an int with ==, and not an object with equals.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you explain why it's better than what the OP does? Specifically, do you feel all color comparisons should be done this way? Why so? \$\endgroup\$ – Pimgd Aug 4 '14 at 7:50
  • \$\begingroup\$ (255,0,0) and (0,255,0) and (0,0,255) are three distinctly different colors whose RGB values all sum up to the same number. \$\endgroup\$ – nhgrif Aug 4 '14 at 11:25

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