Static Final String to Enum

[newcoder310]

I have a class where there are a few static final String variables that have few values that repesent color values. The class is basically used to get the color values. Is it better to convert this to enum?

public class Color{
    public static final String GRAY50 = "#808080";
    public static final String BLACK = "#000000";

    private final String backgroudColor;

    public static Color createBasicColor(){
        return new Color(GRAY50);
    }

    public static Color createSubColor(){
        return new Color(BLACK);
    }

    private Color(String defaultColor){
        this.backgroundColor = defaultColor;
    }

    public String getColor(String color){
        if(color.equals(GRAY50){
            return backgroundColor;
        }
        return color;
    }
}

[stultuske]

it would be a possibility. If you are going to do this, I would recommend to change the

public String getColor(String color){
        if(color.equals(GRAY50){
            return backgroundColor;
        }
        return color;
    }

part and other stuff linking the values and the actual enum, by what Joshua Bloch recommends in his book: "Effective Java, 2nd edition"

[newcoder310]

Kindly, suggest the necessary modifications

public enum Color {

    GRAY50("#808080"), GRAY25("#404040"), BLACK("#000000");

    private final String backgroundColor;

    public static Color createSubjectAreaDefaultColorSchema() {
        return GRAY50;
    }

    public static Color createSubSubjectAreaDefaultColorSchema() {
        return GRAY25;
    }

    private Color(String defaultBackgroundColor) {
        this.backgroundColor = defaultBackgroundColor;
    }

    @Override
    public String toString() {
        return backgroundColor;
    }

    public String getBackgroundColor(String color) {
        if (color.equals(Color.GRAY50.toString())) {
             return backgroundColor;
        }
        return color;
    }

    public String getTextColor(String color) {
        return color;
    }

}

[newcoder310]

 Changed to.
 public String getBackgroundColor(String color) {
if (color.equals(GRAY50.toString())) {
return backgroundColor;
}
return color;
}

[JamesCherrill]

Before people write too much code...
What is the purpose of this class? Does it just hold some standard colors for yor app, and/or do you want to be able to retrive those colors by a name which is a variable at run time?
Colors are immutable, so you don't need accessors etc, just declare the Color variables as public final
If you want to retrieve them by name, build a private Map<String, Color> and populate it, then the public COlor getColorByName(String name) method is a simple get, regardless of the number of available colors.

[newcoder310]

If that is the case then what about createSubSubjectAreaDefaultColorSchema() and createSubjectAreaDefaultColorSchema. I need to return Color object based on that.

[JamesCherrill]

If they take no parameters, then just return the appropriate Color like the examples in the enum code above.
But since those methods don't seem to create anything, let alone a complete schema, they are badly named. And if so, "SubjectAreaDefaultColor" can be just another color name in the Map or value in the enum.

re enums: it's probable that an enum will serve very well for what you are doing, and will provide type safety in method calls. But without a complete spec for what you need to achieve, it's impossible to say - eg if these colors need to be loaded from a user config or preferences file then enum's won't fit.

[newcoder310]

re enums: it's probable that an enum will serve very well for what you are doing, and will provide type safety in method calls. But without a complete spec for what you need to achieve, it's impossible to say - eg if these colors need to be loaded from a user config or preferences file then enum's won't fit.

public String getBackgroundColor(String color) where color is passed from a list of colors at runtime. We match against 3 default colors and based on the if logic given above, return the color.

[newcoder310]

Sorry, my bad. Only one color is passed and matched againt the 3. The list is for a different object which is usually a few.

[JamesCherrill]

Sorry man, I cannot understand what you just posted.

Why does a method called getBackgroundColor return a String? Why does it need a parameter? What is the significance of the String you pass into it?

[newcoder310]

Background color differs depending on level. However, DB default is GRAY50, so we return background color based on context. If GRAY50 use the context default otherwise use the given value.

 public String getBackgroundColor(String color) {
        if (color.equals(GRAY50.toString())) {
            return backgroundColor;
        }
        return color;
    }

[JamesCherrill]

SO the intent is to replace ocurrences of GRAY50 with the background color (which may been defined by a previous call to the (invalid) method called Color(String) on line 15, or maybe is uninitialised). Why not just use the background color in the first place instaed of GRAY50?
And why return a String from a method that gets a Color?

It would be really helpful if you could explain what this class is intended to do, rather than just looking at individual methods that make no sense out of context.

[newcoder310]

so, getting back to the origins. This is being done inorder to populate the properties of a report. There are a bunch of them like pagesize, groupby, summary, fields etc. Now each of the fields has information like name, width, text color, background key color etc. Fields can either have main areas or sub areas. so GRAY50 corresponds to the colorscheme of main area and GRAY25 corresponds to the colorscheme of the sub area. If you see the first post, you can find the inital class that was in place for getting the color for a field in the report. Hope you have understood the context now.

[newcoder310]

hey the previous post somehow became a code post. Can you please edit.

[stultuske]

No. Normally, you could have, for the first x minutes after posting it.

[newcoder310]

How?

[JamesCherrill]

OK.
So all really you want is a getColor method to wrap some kind of Map<String, Color> so you can do stuff like

myField.setBackground(getColor("MyFieldMainArea"));

or maybe

myField.setBackground(getColor("MyField", MAIN_AREA));

<I have to go out now, so no more posts for at least couple of hours... stultuske, you still there mate? - can you take over?>

[newcoder310]

Thanks James!!
Stultuske/James - So now the question is why not enum. Isn't it better to use enum instead of a class especially when the values are limited. Isn't having a map for just 3 fields unnecessary? If having enum is good, then can you please check the previous post where I posted the code and let me know if there is any modification needed.

[newcoder310]

I think there is something wrong with the post. I see that its last been updated by JamesCherrill but, I was the one who updated last.

[newcoder310]

Yeah, Its good now

[JamesCherrill]

Yes, I edited your post so it wasn't code, as requested.

Here's the thing about enums: they are fixed at compile time. So if your colors are hard-wired into the program, and you're happy to re-compile and redistribute your program for any color changes then enums are best.
But of you want colors to be configurable in any way then they will have to change at runtime and enums are not suitable (well, there are ways round that, but it's missing the point).
My guess is that sooner or later someone will want to change those colors, so you will need to read them from a config file or a user Preferences. That's where a simple Map implementation is by far the easiest way to go.

If you want the deluxe solution I would suggest an enum for the various types of color, eg

public enum ColorType {MAIN_BACKGROUND, SUB_BACKGROUND, NORMAL TEXT, ERROR_TEXT etc

then use those as the keys for a Map that links them to actual instances of the Color class, wrapped in a safe method like

public static Color getColor(ColorType ct) {
  return colorMap.get(ct);
}

That gives you all the type safety and self-documenting of an enum, but with the ability to configure the actual Colors that each type should use.

[newcoder310]

But of you want colors to be configurable in any way then they will have to change at runtime and enums are not suitable (well, there are ways round that, but it's missing the point).

So for dragging this a little but I wanted to get a complete understanding. I didn't get what you meant by "configurable in any way". Did you mean this with respect to the color that is set to the field? Or did you mean something else?

And with respect to the number of colors, as far as I know there are only 3 colors as of now as shown in the code earlier. A couple of shades of grey and one shade of black. Hence, I was reluctant to use the map.

[JamesCherrill]

I meant, for example, allowing the user to chose his own preferred colors for his application. Or having different color "themes" that he can chose from. Or having different day/night colors.

With only 3 colors maybe if tests or a switch are as good as a Map, but I try to suggest solutions that are scalable and extendable and which relate better to what happens in real life. The Map approach makes it easy to change colors and allows for any number of color types, so that's what I would use.

[newcoder310]

That might not fit the bill here but its always nice to hear good tips. Thanks James!!