3
\$\begingroup\$

Ok, this might be a very stupid question/idea (I'm still a beginner). Lets say I have a construct like this:

g.setColor(new Color(random.nextInt(5) * speed, 255, speed * 3));

Now it is very likely that the passed arguments will in some cases be out of the expected range, so the code will throw an exception. To circumvent this case I wrote the following helper class:

public class SafeColor {

    static final int INT_MIN = 0;
    static final int INT_MAX = 255;
    static final float FLOAT_MIN = 0;
    static final float FLOAT_MAX = 1;

    public static Color safeIntColor(int r, int g, int b) {
        if (r < INT_MIN) r = INT_MIN;
        if (g < INT_MIN) g = INT_MIN;
        if (b < INT_MIN) b = INT_MIN;

        if (r > INT_MAX) r = INT_MAX;
        if (g > INT_MAX) g = INT_MAX;
        if (b > INT_MAX) b = INT_MAX;

        return new Color(r, g, b);
    }

    public static Color safeIntAlphaColor(int r, int g, int b, int a) {
        if (r < INT_MIN) r = INT_MIN;
        if (g < INT_MIN) g = INT_MIN;
        if (b < INT_MIN) b = INT_MIN;
        if (a < INT_MIN) a = INT_MIN;

        if (r > INT_MAX) r = INT_MAX;
        if (g > INT_MAX) g = INT_MAX;

        // ... and so on ...

Now I can do this:

g.setColor(SafeColor.safeIntColor(random.nextInt(5) * speed, 255, speed * 3));

I guess the question is, does that make any sense? The whole thing looks a bit overdone to me. Is there a better/easier way to accomplish the same thing?


Edit: For anyone interested. I boiled it down to this:

public class SafeColor {

    private static final float FLOAT_MAX = 1;
    private static final float FLOAT_MIN = 0;
    private static final int INT_MAX = 255;
    private static final int INT_MIN = 0;

    private static float safeFloat(float f) {
        return Math.max(Math.min(f, FLOAT_MAX), FLOAT_MIN);
    }

    private static int safeInt(int i) {
        return Math.max(Math.min(i, INT_MAX), INT_MIN);
    }

    public static Color safeColor(float r, float g, float b, float a) {
        return new Color(safeFloat(r), safeFloat(g), safeFloat(b), safeFloat(a));
    }

    public static Color safeColor(float r, float g, float b) {
        return new Color(safeFloat(r), safeFloat(g), safeFloat(b));
    }

    public static Color safeColor(int r, int g, int b, int a) {
        return new Color(safeInt(r), safeInt(g), safeInt(b), safeInt(a));
    }

    public static Color safeColor(int r, int g, int b) {
        return new Color(safeInt(r), safeInt(g), safeInt(b));
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ It is a fine thing to do; nice job, in fact! I would not change a thing. \$\endgroup\$ – Ernest Friedman-Hill Jul 26 '13 at 19:36
  • 1
    \$\begingroup\$ Nothing wrong about validation. I'd just use Math.max() and Math.min() to make the intent more clear. \$\endgroup\$ – kiheru Jul 26 '13 at 19:39
5
\$\begingroup\$

There is nothing too bad about this, and it is an acceptable pattern to get rid of extra/boilerplate code (for example IOUtils from Apache Commons has a closeQuietly for streams that takes a way a lot of the nested try-catch-finally madness for closing streams).

However, I would argue that it is better to validate your inputs to make sure that they don't exceed the ranges, or at least change the code that generates your values to ensure that they do not generate invalid values.

For example, you could have a method called generateColor(speed) that returns a sane Color instance.

I would also suggest using Math.max and Math.min in this method instead of using a bunch of ifs.

\$\endgroup\$

Your Answer

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