4
\$\begingroup\$

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:

  1. String is about to to be painted. What color should it be?
  2. chooseColor(4);
  3. chooseColor(4) = BelizeHole. Let's paint it with that color.
\$\endgroup\$
  • \$\begingroup\$ How is this method called? In what context is it used? \$\endgroup\$ – I'll add comments tomorrow May 31 '16 at 10:14
  • \$\begingroup\$ Forgive me. Should have given more info. Edited the Question. :) \$\endgroup\$ – Azazel May 31 '16 at 10:15
  • \$\begingroup\$ I wasn't referring to what's going on inside the method, but rather outside of it. How and where do you call that method? In what situations is it used? How are the values that you pass to it determined? \$\endgroup\$ – I'll add comments tomorrow May 31 '16 at 10:30
  • \$\begingroup\$ How is the i given as parameter determined? Where is it coming from? Could you post an example of calling code? \$\endgroup\$ – Tunaki May 31 '16 at 11:20
  • 1
    \$\begingroup\$ Just a thought, since this is a 2048 clone, why not just operate on powers of two? Instead of having i as 2, 4... 2048, why not have it as 1, 2... 11? \$\endgroup\$ – h.j.k. May 31 '16 at 13:56
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Thankyou very much for the improvements you suggested. I understand what you mean by the magic values comment and it was irking me as well. I knew something was wrong there but wasn;t sure how to fix it. This may be one way. :) \$\endgroup\$ – Azazel Jun 1 '16 at 11:39
3
\$\begingroup\$

Having explicit names for the Color objects is a problem.

Color Emerald = new Color(231, 223, 134);

The name should certainly be descriptive, but what if you change the values of the channels of the color at some point? You should rename the variable to reflect the new value, but nothing enforces that.

These names are great for constants like the public static members of the Color class, but not so much in your situation, because the values are subject to change.

But I'm getting ahead of myself, let's start at the beginning.

know your data

Start with modelling the data for your problem in a convenient way.

Its a 2048 clone and that string is going to be one of the numbers like 2, 4, 8, 16, 32

Sure enough, that works, but as @h.j.k. pointed out:

since this is a 2048 clone, why not just operate on powers of two? Instead of having i as 2, 4... 2048, why not have it as 1, 2... 11?

I think that's spot on. Just because you display numbers, doesn't mean that you should store them in exactly the same way. This is why they always say keep data and display separate.

data structure

If you want to model the numbers as their actual values or the powers of two that they are, either way: you want to use a data structure that makes it easy to associate a value with a color in both cases.

I mean just look at these numbers in the comments:

...3, 134); //2
.......04); // 4
......, 3);// 8

They are dying to be of actual use and not just documentation!

To associate the values (2, 4...) with their respective colors, a HashMap is very useful. It looks like converting a value to a Color is static functionality, that's only based on the value and always gives the same output for the same value. It's more of a static utility functionality than a non-static class member. Such a ColorConverter class could look like that:

import java.util.HashMap;
import java.awt.Color;

public class ColorConverter
{
    private ColorConverter()
    {
        // nope!
    }

    private static final HashMap <Integer, Color> valueToColor;
    private static final Color defaultColor;

    static
    {
        defaultColor = Color.BLACK;

        valueToColor = new HashMap <Integer, Color>();
        valueToColor.put(2, new Color(231, 223, 134));
        valueToColor.put(4, new Color(151, 206, 104));
        valueToColor.put(8, new Color(240, 79, 3));
        valueToColor.put(16, new Color(116, 116, 204));
        valueToColor.put(32, new Color(89, 188, 251));
        valueToColor.put(64, new Color(245, 213, 69));
        valueToColor.put(128, new Color(46, 204, 113));
        valueToColor.put(256, new Color(254, 198, 6));
        valueToColor.put(512, new Color(151, 206, 104));
        valueToColor.put(1024, new Color(136, 112, 255));       
        valueToColor.put(2048, new Color(255, 215, 0));
    }

    public static Color toColor(int value)
    {
        return valueToColor.containsKey(value) ? valueToColor.get(value) : defaultColor;
    }
}

Adding a tiny main method for testing

public static void main(String[] args)
{
    System.out.println(ColorConverter.toColor(2));
    System.out.println(ColorConverter.toColor(64));
    System.out.println(ColorConverter.toColor(527));
}

yields this result when executed in the command line:

java.awt.Color[r=231,g=223,b=134]
java.awt.Color[r=245,g=213,b=69]
java.awt.Color[r=0,g=0,b=0]

To associate the powers of 2 (1, 2,...) with their respective colors, you could also use a HashMap or even an Array, if you include the the power 0, which corresponds to a value of 1, because the values are consecutive integers, which works well with an array that has indices that are consecutive integers. For brevity, I do not include the code for that, because it's quite similar to the above. If you need it, please leave a comment.

\$\endgroup\$
  • \$\begingroup\$ Thankyou so much. I am a beginner and not the most smartest of what to use in what areas at times so its very useful to learn what may be best. I will be doing my best to take your help in and try to work this into my code. :) Thankyou again. \$\endgroup\$ – Azazel Jun 1 '16 at 11:38
1
\$\begingroup\$

I think there are two more efficient ways to do this, both focusing on the fact that the variables are initialized every time the function is called. One is just to use a HashMap in the class containing the function, and doing a get. The other way is to refactor it out into an enum:

public enum Colors {
    EMERALD       (new Color(231, 223, 134)),
    BELIZE_HOLE   (new Color(151, 206, 104)),
    CARROT        (new Color(240,  79,   3)),
    MIDNIGHT_BLUE (new Color(116, 116, 204)),
    SUNFLOWER     (new Color( 89, 188, 251)),
    ALIZARIN      (new Color(245, 213,  69)),
    WISTERIA      (new Color( 46, 204, 113)),
    SILVER        (new Color(254, 198,   6)),
    CONCRETE      (new Color(151, 206, 104)),
    ORANGE        (new Color(136, 112, 255)),
    AMETHYST      (new Color(255, 215,   0))

    private HashMap<int, Colors> Lookup = new HashMap<int, Colors>(){{
        put(   1,       EMERALD);
        put(   2,   BELIZE_HOLE);
        put(   4,        CARROT);
        put(   8, MIDNIGHT_BLUE);
        put(  16,     SUNFLOWER);
        put(  32,      ALIZARIN);
        put(  64,      WISTERIA);
        put( 128,        SILVER);
        put( 256,      CONCRETE);
        put(1024,        ORANGE);
        put(2048,      AMETHYST);
    }}

    public static Colors fromNumber(int number) {
        return Lookup.get(number);
    }
}

This way, your color code and data are separate from the main logic, and also from the main class. You should be able to get the color by doing Colors.fromNumber(number).

\$\endgroup\$
  • \$\begingroup\$ This looks straight forward. Maybe one improvement can be made. You could calculate the key by 2 power of size(). \$\endgroup\$ – oopexpert May 31 '16 at 16:28
  • \$\begingroup\$ What doo you mean bysize()? \$\endgroup\$ – somebody May 31 '16 at 22:36
  • \$\begingroup\$ the size of the map. put(EMERALD, (int) Math.pow(2.0, size()); \$\endgroup\$ – oopexpert May 31 '16 at 22:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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