Choosing a Color based on an int for a 2048 clone in Java

Is it possible to make this cleaner / more efficient? I know that this is probably not the best way to do this and probably could be made better but I cannot figure out how I do that.

Basically, a number is given which is then compared to the numbers in Num[] and then returns a color based on that comparison.

private Color chooseColor(int i){
    Color Emerald = new Color(231, 223, 134); //2
    Color BelizeHole = new Color(151, 206, 104); // 4
    Color Carrot = new Color(240, 79, 3);// 8
    Color MidnightBlue = new Color(116, 116, 204); // 16
    Color SunFlower = new Color(89, 188, 251);// 32
    Color Alizarin = new Color(245, 213, 69);// 64
    Color Wisteria = new Color(46, 204, 113); // 128
    Color Silver = new Color(254, 198, 6); //256
    Color Concrete = new Color(151, 206, 104); //512
    Color Orange = new Color(136, 112, 255); //1024
    Color Amethyst = new Color(255, 215, 0); //2048
    Color[] RECT_COLORS = {Emerald, BelizeHole, Carrot, Amethyst, SunFlower, Alizarin, Wisteria, Silver, Concrete, Orange, Amethyst};
    int[] Num = {2,4,8,16,32,64,128,256,512,1024,2048};

    for(int j = 0 ; j < Num.length ; j++){
        if(i == Num[j]){
            return RECT_COLORS[j];
        } else if ( j == Num.length - 1 ){
            return RECT_COLORS[Num.length-1];       
        }
    }

    return RECT_COLORS[Num.length-1];
}

This compiles and runs fine.

Outside of the method, it is called when painting a string on Canvas and returns a color based on the string value which is what color the string is then painted as.

Basically:

• String is about to to be painted. What color should it be?

• chooseColor(4);

• chooseColor(4) = BelizeHole. Let's paint it with that color.

Something is wrong with your design. Your method is technically correct, but the problem is that it relies on a given magic int i to determine which color to return.

This can be seen by

int[] Num = {2,4,8,16,32,64,128,256,512,1024,2048};

This declares an array of fixed magic values. The issue is that, somewhere in your code, you're determining which i to give to this method and you're testing it against known fixed values only in the method.

This highly couples parts of your application. You should refactor this code so that the logic of "which number corresponds to which color" is centralized in a single place. One of comment says that you're doing a 2048 clone, as such, the centralized place could be a class called Block. You could even make it an enumeration of all the possible blocks, each with its respective value and color.

As such, don't use use Strings to represent blocks, use proper objects with attributes; this is what Object-Oriented Programming is about. An example could be the following (with perhaps a better name for the constants):

public enum Block {

    SQUARE_2(2, new Color(231, 223, 134)),
    SQUARE_4(4, new Color(151, 206, 104));

    private final int value;
    private final Color color;

    private Block(int value, Color color) {
        this.value = value;
        this.color = color;
    }

    // add getters

}

That said, you can improve a little bit the current code. You are currently doing:

for(int j = 0 ; j < Num.length ; j++){
    if(i == Num[j]){
        return RECT_COLORS[j];
    } else if ( j == Num.length - 1 ){
        return RECT_COLORS[Num.length-1];       
    }
}
return RECT_COLORS[Num.length-1];

In the case where the Num array doesn't have the given value, you have a special else if to handle the last case. You actually don't need this: you can safely let your code exit the for loop normally and return the last value in this case:

for (int j = 0 ; j < Num.length ; j++) {
    if (i == Num[j]) {
        return RECT_COLORS[j];
    }
}
return RECT_COLORS[Num.length-1];

A second point concerns the variable names: you should try to respect Java naming conventions. The local variables should start with a lowercase letter (num instead of Num, which could arguably have a better name also; emerald instead of Emerald...).

StackExchange Code Review Q#129749, answer score: 4