Skip to main content
2 of 3
added 818 characters in body
nickb
  • 221
  • 2
  • 7

Tunaki did a great job on your main code, I have one additional comment on how to rewrite your getName() method:

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

This makes it a little more clear on your stopping intentions - You're done with the loop as soon as the name you generate isn't in the list of generated names. It's also a perfect use-case for a do...while loop. You must do something at least once!


I also have a few suggestions for your test code:

Validation Bug

Your regex does not have anchors, so it will match any part of the string when you check for matches. For example, this:

System.out.println("123AA123ZZ".matches("[A-Z]{2}\\d{3}"));

Outputs true! Add a start ^ and ending $ anchor in there to make sure the regex needs to match the whole string:

"^[A-Z]{2}\\d{3}$"

Define and reuse a Pattern object

You're defining a regex with EXPECTED_ROBOT_NAME_PATTERN and even named it Pattern, might as well compile it and save a reference to an actual Pattern object. Otherwise, when you call String#matches, it has to compile it every time:

private static final Pattern EXPECTED_ROBOT_NAME_PATTERN = Pattern.compile("^[A-Z]{2}\\d{3}$");

You then use it by seeing if it matches:

boolean matches = EXPECTED_ROBOT_NAME_PATTERN.matcher(name).matches();

Unnecessary assertThat

Typically you'll use assertThat in a declarative style to make the code more readable, but your current assertion doesn't read well and isn't very clear on what you're testing for. You can simply use assertTrue, which I think is simpler in this case:

private static void assertIsValidName(String name) {
    boolean matches = EXPECTED_ROBOT_NAME_PATTERN.matcher(name).matches();
    assertTrue("name does not match expected pattern", matches);
}

Note that because I switched to a Pattern object and am using Matcher#matches to validate the String, I could omit the anchors that I suggested to put in, since Matcher#matches checks against the whole string.

nickb
  • 221
  • 2
  • 7