Software Key Generator

I have created a Java program which generates a key (for activating a piece of software for example) my code works correctly however there are parts of it that I know can be improved. In particular inside the createKey method were I have repeated an if statement in multiple places.

I would appreciate if you can take some time to look at and spots ways to improve my code.I would rather learn first and foremost ways to refine the program in its current form. Also, if you see any bad habits, let me know.

I plan to save each key to a file and build a validator that allows input and references the file.


Description


Creates a key of 20 letters and numbers - the letters and numbers are
set a weighting, if two is returned as a weight for a letter and
three for numbers.


AA232BC433 ... so two letters then three numbers and so on.


10 numbers and 10 letters are then selected.


Each is chosen through a loop that runs 10 times and randomly assigns
a value from each final array into a set of 'chosen' values.


The create key method then cycles through various loops which use the
weights to add a further random selection of the chosen values -
checks are added which ensure that the string builder stops adding new
values when the length is 20.

public class KeyTestDrive {

public static void main(String[] args) {
    Key key = new Key();
    key.createKey();
}

}

```
import java.util.Random;

public class Key {

private final int[] possibleNumbers = { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
private final char[] possibleLetters = { 'A', 'B', 'C', 'D', 'E', 'F', 'G',
'H', 'I', 'J', 'K', 'L', 'M', 'O', 'P', 'Q', 'R', 'S', 'T', 'U',
'V', 'W', 'X', 'Y', 'Z' };

Random randomNum = new Random();
StringBuilder key = new StringBuilder();

public void createKey() {
int letterWeighting = determineLetterWeigthing();
int numberWeighting = determineNumberWeigthing();

int[] chosenNumbers = selectNumbers();
char[] chosen

Improving createKey

First of all, when the same couple of lines appear duplicated several times, there's clearly a problem. I'm talking about this one:

if(isSizeTwenty == true){
    break;
}

A good first step is to try to eliminate this. Consider this situation:

boolean isSizeTwenty = false;

for (int i = 0; i < 20; i++) {

    if(isSizeTwenty == true){
        break;
    }

    // ...

    if(isSizeTwenty == true){
        break;
    }
}

Notice that the first check inside the loop will be always false: isSizeTwenty is initialized with false, and since you have this check as the last step of the loop, the condition can never be true at the beginning of the loop.

Also, don't write conditions on boolean expressions like this:

if (someboolean == true) { ... }

You can write simpler and better like this:

if (someboolean) { ... }

But, the entire method can be refactored far better, for example:

public String createKey() {
    int letterWeighting = determineLetterWeigthing();
    int numberWeighting = determineNumberWeigthing();

    int[] chosenNumbers = selectNumbers();
    char[] chosenLetters = selectLetters();

    StringBuilder key = new StringBuilder();

    while (key.length() < targetLength) {
        for (int j = 0; j < letterWeighting && key.length() < targetLength; j++) {
            int randomIndex = random.nextInt(chosenLetters.length - 1);
            key.append(chosenLetters[randomIndex]);
        }
        for (int k = 0; k < numberWeighting && key.length() < targetLength; k++) {
            int randomIndex = random.nextInt(chosenNumbers.length - 1);
            key.append(chosenNumbers[randomIndex]);
        }
    }
    return key.toString();
}

I made a couple of changes there:

• Replaced the number 20 with a meaningful variable named targetLength

• Removed of the isSizeTwenty variable, and moved the condition key.length()






• Moved StringBuilder inside the method. No need to make that a member variable






• Replaced the outermost for (int i; i

• Replaced the hardcoded number 9 with chosenLetters.length - 1 and chosenNumbers.length - 1. This is more than just about removing another magic number, this is also safer, because now randomIndex can never be accidentally out of bounds. Now it is derived from the length of the available letters and numbers.

• Renamed randomNum to just random, because there is nothing "num"-like about an instance of Random

Improving determineLetterWeigthing and determineNumberWeigthing

You could reduce these methods to single lines:

private int determineLetterWeigthing() {
    return random.nextInt(4) + 1;
}

private int determineNumberWeigthing() {
    return random.nextInt(4) + 1;
}

Unit testing

I would make random and targetLength final members, and add these constructors:

private final Random random;
private final int targetLength;

public Key(int targetLength) {
    this(targetLength, new Random());
}

public Key(int targetLength, Random random) {
    this.random = random;
    this.targetLength = targetLength;
}

This way, and thanks to the new version of createKey that returns the key instead of printing it, now you can add some unit tests:

@Test
public void testExampleKeys() {
    assertEquals("VAT3735TVV4949RVJ793", new Key(20, new Random(0)).createKey());
    assertEquals("SBM8PBJ8PSJ6MWS2SBO2", new Key(20, new Random(1)).createKey());
    assertEquals("DGG77DJS28SDP13GGD77", new Key(20, new Random(2)).createKey());
    assertEquals("PGB844MGM477GCG728PL", new Key(20, new Random(3)).createKey());
}

@Test
public void testCreateMultipleKeysFromOneGenerator() {
    Key keygen = new Key(20, new Random(10));
    assertEquals("YTD15DTT52YTO12IRL21", keygen.createKey());
    assertEquals("IK617LP637LG611IK619", keygen.createKey());
    assertEquals("VH1WY5YO1YP8SO9YO1YW", keygen.createKey());
}

Actually, it would be better to rename your class to KeyGenerator.
Notice the 2nd unit test above: I called the instance keygen, which is just natural, and changing the class name to match would be natural too.

Other things

An easier way to create possibleLetters:

private final char[] possibleLetters = "ABCDEFGHIJKLMOPQRSTUVWXYZ".toCharArray();

Btw, do you have something against the letter "N"? It wasn't on your list :)

In selectLetters and selectNumbers, 10 is a magic number:

char[] chosenLetters = new char[10];

It would be better to make it a private static final constant at the top of the class with a good name.

Actually it would be better to make selectLetters and selectNumbers take a parameter.

StackExchange Code Review Q#63279, answer score: 3