Generating Robot Name Problem:


  Write a program that manages robot factory settings.
  
  When robots come off the factory floor, they have no name.
  
  The first time you boot them up, a random name is generated, such as
  RX837 or BC811.
  
  Every once in a while we need to reset a robot to its factory
  settings, which means that their name gets wiped. The next time you
  ask, it will respond with a new random name.
  
  The names must be random: they should not follow a predictable
  sequence. Random names means a risk of collisions. Your solution
  should not allow the use of the same name twice when avoidable. In
  some exercism language tracks there are tests to ensure that the same
  name is never used twice.


Code:

import java.util.Random;
import java.util.Set;
import java.util.HashSet;

public class Robot {
  private static final String ALPHABETS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
  private static final Set&lt;String&gt; generatedNames = new HashSet&lt;&gt;();
  private String name;

  public String getName() {
    while (name == null) {
      name = generateName();
      if (generatedNames.contains(name)) name = null;
    }
    generatedNames.add(name);
    return name; 
  }

  public void reset() {
    name = null;    
  }

  private String generateName() {
    StringBuilder builder = new StringBuilder();
    int i;
    for (i = 0; i &lt; 2; i++) {
      builder.append(ALPHABETS.charAt(generateRandomInRange(0, 25)));
    }
    for (i = 0; i &lt; 3; i++) {
      builder.append(generateRandomInRange(0, 9));
    }
    return builder.toString();
  }

  private int generateRandomInRange(int min, int max) {
    return new Random().nextInt((max - min) + 1) + min;
  }
}


Test Suite:

import org.junit.Test;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.IsNot.not;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertEquals;

public class RobotTest {

    private static final String EXPECTED_ROBOT_NAME_PATTERN = "[A-Z]{2}\\d{3}";
    private final Robot robot = new Robot();

    @Test
    public void hasName() {
        assertIsValidName(robot.getName());
    }

    @Test
    public void differentRobotsHaveDifferentNames() {
        assertThat(robot.getName(), not(equalTo(new Robot().getName())));
    }

    @Test
    public void resetName() {
        final String name = robot.getName();
        robot.reset();
        final String name2 = robot.getName();
        assertThat(name, not(equalTo(name2)));
        assertIsValidName(name2);
    }

    private static void assertIsValidName(String name) {
        assertThat(name.matches(EXPECTED_ROBOT_NAME_PATTERN), is(true));
    }
}


Question:

Apart my naming and data structure I am interested in knowing if my solution would scale well or not?

Reference: Exercism
Quality of random numbers

You are generating a random integer in a range with the method

private int generateRandomInRange(int min, int max) {
  return new Random().nextInt((max - min) + 1) + min;
}


This method creates a new Random object each time it is called. This is not a good idea. A random number generator should only be instantiated once and be reused throughout the application.

There is a FindBugs warning for exactly that (emphasis mine):


  This code creates a java.util.Random object, uses it to generate one random number, and then discards the Random object. This produces mediocre quality random numbers and is inefficient.


What you want instead is to have a single Random object. You could create it as a constant and then reuse it, like this:

private static final Random RANDOM = new Random();

private int generateRandomInRange(int min, int max) {
    return RANDOM.nextInt((max - min) + 1) + min;
}


But, in this case, since you're using it to return a random integer in a range, it would be even simpler to use the built-in ThreadLocalRandom class:

private int generateRandomInRange(int min, int max) {
    return ThreadLocalRandom.current().nextInt(min, max + 1);
}


This will directly return a random integer in the range [min, max].

Pre-allocating the StringBuilder

Small nit-pick, but you're using 

StringBuilder builder = new StringBuilder();


to instantiate the StringBuilder. Since we know that the result will have 5 characters, we might as well initialize it with that:

StringBuilder builder = new StringBuilder(5);


It will create in memory an array of 5 characters instead of 16 (which is the default). As commented by Insane, you could extract that 5 (along with the 2 and 3 in the for loop) into a constant (named, for example, ROBOT_NAME_LENGTH).

Variable scope

In your generateName method, you have:

int i;
for (i = 0; i &lt; 2; i++) {
  builder.append(ALPHABETS.charAt(generateRandomInRange(0, 25)));
}
for (i = 0; i &lt; 3; i++) {
  builder.append(generateRandomInRange(0, 9));
}


which first declares i and uses it afterwards. There is no need to do that and it is a lot clearer to just have

for (int i = 0; i &lt; 2; i++) {
  builder.append(ALPHABETS.charAt(generateRandomInRange(0, 25)));
}
for (int i = 0; i &lt; 3; i++) {
  builder.append(generateRandomInRange(0, 9));
}


About the unicity of the names

Currently, you are storing all the generated names inside a Set. This is great because you can then check in constant-time if a given name was already generated. The small hiccup is that you will keep generating names until you find one that hasn't been generated before.

I don't see an easy way to optimize that or do better. The brute-force solution of generating all possible names up-front is possible in this case (since there are "only" 26 * 26 * 10 * 10 * 10 = 676000 possibilites) but not very pretty, and it wouldn't scale well at all.

Your solution is the best with regard to difficulty to write / understand versus the time it takes to generate the name. I tested with by generating a name for all the 676000 possible robots and it did it in a couple of seconds.

However, it leads to a possible bug.

Possible bug

There are only 676000 possible names. This means that if you want to name the robot 676001, your code will loop forever: it will try again and again to generate a name and will never terminate since they have all been generated.

A possible solution is to test whether all the names have been generated: if it is the case, you can simply clear the generated names set (remove all values). The problem description says:


  Your solution should not allow the use of the same name twice when avoidable.


In the case when they have all been generated, it is unavoidable, and one solution could be to decide to start again from 0. Another solution could also be to simply return one by one the already generated names (since we know they are unique) and not generate them again.

Oh, and also, generatedNames should be named GENERATED_NAMES since it is declared as static final.