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
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
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.