6
\$\begingroup\$

I'd want to see if is possible to optimize performance even further of the following generate color method.

package generacolorrgb;

import java.util.Random;

public class GeneracolorRGB {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        Random r = new Random();
        for (int i = 0; i < 100; i++) {
            System.out.println(generateColor(r));
        }

    }

    private static String generateColor(Random r) {
        StringBuilder color = new StringBuilder(Integer.toHexString(r
                .nextInt(16777215)));
        while (color.length() < 6) {
            color.append("0");
        }

        return color.append("#").reverse().toString();

    }

}

Average time it takes on my machine: 1650704.333 ns

\$\endgroup\$
1
  • \$\begingroup\$ If you want to test it with @Zymus code it takes 93.3 nanoseconds average \$\endgroup\$ Oct 23, 2015 at 23:09

3 Answers 3

2
\$\begingroup\$

Faster implementation

If you build the string yourself using a character array, you can generate the string much faster. The slowest part of your string conversion is probably the call to Integer.toHexString().

I got 2x the speed using this code:

private static String generateColor(Random r) {
    final char [] hex = { '0', '1', '2', '3', '4', '5', '6', '7',
                          '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
    char [] s = new char[7];
    int     n = r.nextInt(0x1000000);

    s[0] = '#';
    for (int i=1;i<7;i++) {
        s[i] = hex[n & 0xf];
        n >>= 4;
    }
    return new String(s);
}

I ran 10000000 iterations to test the timing vs the original code.

\$\endgroup\$
4
  • \$\begingroup\$ An arguable more expressive version of creating the char[] hex array: "0123456789abcdef".toCharArray(). :) \$\endgroup\$
    – h.j.k.
    Oct 25, 2015 at 8:42
  • 1
    \$\begingroup\$ @h.j.k. I'm not a java expert so I don't know what's best. I tried to write something that would be constructed at compile time instead of at runtime. Are our two versions equivalent? \$\endgroup\$
    – JS1
    Oct 25, 2015 at 9:00
  • \$\begingroup\$ Actually that's a good point. "string".toCharArray() simply makes a copy of the underlying char[] content of the "string" using System.arraycopy(). It should be quite close performance-wise, especially if we extract hex out as a static final char[] field, for example. \$\endgroup\$
    – h.j.k.
    Oct 25, 2015 at 13:18
  • 1
    \$\begingroup\$ As @h.j.k pointed its practically equivallent. 30.7ns amazing! \$\endgroup\$ Oct 26, 2015 at 15:35
5
\$\begingroup\$

Here are some comments related to your code:

  • Strange place to initialise the Random() – This should either be in within the generateColor() function, or some static initialisation. As it stands now, it looks strange that you need to include the random generator to as a parameter to your function. You are better of using a static initialization
  • Strange magic number, 16777215 – What is this number? It is not very clear that this is actually the 0xFFFFFF denoting the maximum color number. Also note that nextInt(n) returns a number in the range 0 up to n, but not including n. In other words, you should use 0x1000000
  • Tricking into being 6 characters – If you add 0x1000000 to the number before converting to hexadecimal you are ensured six digits, and don't need the trick for 6 characters, and can return a substring (instead of using a while loop)

Applying this and your code looks something like this:

package generacolorrgb;

import java.util.Random;

public class GeneracolorRGB {

    static Random randomGenerator;

    static {
        randomGenerator = new Random();
    }


    public static void main(String[] args) {
        for (int i = 0; i < 100; i++) {
            System.out.println(generateColor());
        }
    }

    private static String generateColor() {
        int newColor = 0x1000000 + randomGenerator.nextInt(0x1000000);
        return "#" + Integer.toHexString(newColor).substring(1, 7);
    }
}

Added: An even nicer variant would be:

    private static String generateColor() {
        int newColor = randomGenerator.nextInt(0x1000000);
        return String.format("#%06X", newColor);
    }

Here we let the Formatter handle the formatting issue, and it does indeed look nicer! And we don't need to make sure the random number is above the legal range, as the formatter handles it.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ "As it stands now, it looks strange that you need to include the random generator to as a parameter to your function" Are you kidding me? That's the best line of code in the original submission. \$\endgroup\$ Oct 23, 2015 at 20:30
  • \$\begingroup\$ @VoiceOfUnreason, Your mileage may vary, but to include a random generator as a function parameter is not good coding in my world. Why would you like to do so? It is part of the intrinsic of the function. \$\endgroup\$
    – holroy
    Oct 23, 2015 at 20:38
  • 1
    \$\begingroup\$ Please don't fight... I specified that the method had to be optimized, because I use this method on a seam project with the random generator initialized as a private field so it's beyond the central subject of the question. \$\endgroup\$ Oct 23, 2015 at 20:43
  • 1
    \$\begingroup\$ @holroy Your optimization Took 1370277 ns Awesome! and the one with the formatter 14158638.33 ns \$\endgroup\$ Oct 23, 2015 at 20:48
  • 1
    \$\begingroup\$ @holroy because trying to write tests with deterministic outcomes becomes ridiculously complicated when methods start instantiating their own instances of RNG. You can pass an RNG with a fixed seed to a method that accepts an RNG. (You are, of course, correct that the RNG is an implementation detail of the generator -- the example would be better off with a constructor that accepts an RNG, and a clean method signature). \$\endgroup\$ Oct 23, 2015 at 21:20
3
\$\begingroup\$

Performance-wise, this is pretty good, though there might be a micro-optimization that I'm not seeing. However, you do have a randomness issue. I ran a sample run of 500M iterations, and these are the color averages I saw:

redAverage=119.970404962
blueAverage=127.471355866
greenAverage=127.994226206

I would propose the following alternative. It performs at about the same speed over 500M iterations, may be slightly easier to understand, and distributes values evenly.

public final class GeneracolorRGB {

    private static final int ITERATIONS = 100;

    private static final String[] VALUES = {
            "0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "A", "B", "C", "D", "E", "F"
    };

    /**
     * @param args the command line arguments
     */
    public static void main(final String[] args) {
        final Random random = new Random();
        for (int i = 0; i < ITERATIONS; i++) {
            System.out.println(generateColor(random));
        }
    }

    private static String generateColor(final Random random) {
        final StringBuilder color = new StringBuilder("#");
        for (int i = 0; i < 6; i++) {
            final String value = VALUES[random.nextInt(VALUES.length)];
            color.append(value);
        }
        return color.toString();
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ The idea behing using number 16777215 was to avoid generate several random numbers and use just one. In such sense I think that StringBuilder color = new StringBuilder("#"); for (int i = 0; i < 6; i++) { color.append(Integer.toHexString(r.nextInt(15))); } will be faster, wich was the original code approach. \$\endgroup\$ Oct 23, 2015 at 19:33
  • \$\begingroup\$ @RuslanLópezCarro Have you tested? I have. The performance is indistinguishable. \$\endgroup\$
    – Eric Stein
    Oct 23, 2015 at 19:35
  • \$\begingroup\$ Yours take 1581960.667 ns on my machine... as you said no big difference. \$\endgroup\$ Oct 23, 2015 at 20:38
  • \$\begingroup\$ Great analysis with the randomness bias. The OP's code looks so unsuspicious that this pattern could be a good candidate for IOCCC-style competitions. \$\endgroup\$ Jun 18, 2021 at 16:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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