Skip to main content
Code Review

Return to Answer

with improved dictionary
Source Link
rolfl
rolfl
  • 98.2k
  • 17
  • 220
  • 419

Your DICTIONARY should be an array of chars, not an array of String. Actually, I quite like just the String option...

private static final Stringchar[] DICTIONARYDIGITS = ("0123456789"
       + "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
       + "abcdefghijklmnopqrstuvwxzy";"abcdefghijklmnopqrstuvwxyz").toCharArray();
private static final int MaxNumberBASE = 238328;DIGITS.length;
private static final int baseMAX_NUMBER = DICTIONARY.length();BASE * BASE * BASE;

public static String generateCode(int num) {
    if (num < 1 || num > MaxNumberMAX_NUMBER) {
        throw new IllegalArgumentException("Illegal input value: " + num);
    }

    int value = num - 1;

    char ac = DICTIONARY.charAt(DIGITS[((value / baseBASE) / baseBASE) % base);BASE];
    char bc = DICTIONARY.charAt(DIGITS[(value / baseBASE) % base);BASE];
    char cc = DICTIONARY.charAt(valueDIGITS[value % base);BASE];
    return new String(new char[]{ac, bc, cc});
}

Further, when running the code,Further, when running the code, I discovered that you are off on your zzz assertion .... the max value you propose for zzz is wrong, what you have is yyy. - I discovered that you are off on your zzz assertion .... the max value you propose forI had a text transpose of zzzzy is wrong, what you have isinstead of yyyyz in my constant.

See the code running on ideone: https://ideone.com/ZPS6fehttps://ideone.com/EZqXlJ

Value 0 Error Illegal input value: 0
Value 1: 000
Value 2: 001
Value 62: 00z
Value 63: 010
Value 1000: 0G7
Value 238328: yyyzzz
Value 238329 Error Illegal input value: 238329

Your DICTIONARY should be an array of chars, not an array of String. Actually, I quite like just the String option...

private static final String DICTIONARY = "0123456789"
       + "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
       + "abcdefghijklmnopqrstuvwxzy";
private static final int MaxNumber = 238328;
private static final int base = DICTIONARY.length();

public static String generateCode(int num) {
    if (num < 1 || num > MaxNumber) {
        throw new IllegalArgumentException("Illegal input value: " + num);
    }

    int value = num - 1;

    char ac = DICTIONARY.charAt(((value / base) / base) % base);
    char bc = DICTIONARY.charAt((value / base) % base);
    char cc = DICTIONARY.charAt(value % base);
    return new String(new char[]{ac, bc, cc});
}

Further, when running the code, I discovered that you are off on your zzz assertion .... the max value you propose for zzz is wrong, what you have is yyy.

See the code running on ideone: https://ideone.com/ZPS6fe

Value 0 Error Illegal input value: 0
Value 1: 000
Value 2: 001
Value 1000: 0G7
Value 238328: yyy
Value 238329 Error Illegal input value: 238329

Your DICTIONARY should be an array of chars, not an array of String.

private static final char[] DIGITS = ("0123456789"
       + "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
       + "abcdefghijklmnopqrstuvwxyz").toCharArray();
private static final int BASE = DIGITS.length;
private static final int MAX_NUMBER = BASE * BASE * BASE;

public static String generateCode(int num) {
    if (num < 1 || num > MAX_NUMBER) {
        throw new IllegalArgumentException("Illegal input value: " + num);
    }

    int value = num - 1;

    char ac = DIGITS[((value / BASE) / BASE) % BASE];
    char bc = DIGITS[(value / BASE) % BASE];
    char cc = DIGITS[value % BASE];
    return new String(new char[]{ac, bc, cc});
}

Further, when running the code, I discovered that you are off on your zzz assertion .... the max value you propose for zzz is wrong, what you have is yyy. - I discovered I had a text transpose of zy instead of yz in my constant.

See the code running on ideone: https://ideone.com/EZqXlJ

Value 0 Error Illegal input value: 0
Value 1: 000
Value 2: 001
Value 62: 00z
Value 63: 010
Value 1000: 0G7
Value 238328: zzz
Value 238329 Error Illegal input value: 238329
Source Link
rolfl
rolfl
  • 98.2k
  • 17
  • 220
  • 419

Your problem definition is .... odd. your code, will, for the input value 1 generate the output 000. Is that what you really want? Why do you need to 1-index the value set instead of 0-index it?

Further, your code caches all values up-to-and-including the input value in a HashMap. This can become quite large.... for the full dataset you're looking at about a quarter-million values where each is about 128 bytes of memory (a String, an Integer, and a Map.Entry plus some other overheads), or about 70 megabytes of data.

Your DICTIONARY should be an array of chars, not an array of String. Actually, I quite like just the String option...

The maxNumber should be a static constant too.

Finally, you return a null value for an invalid input. That's very unconventional, you should throw an IllegalArgumentException instead.

As mentioned in a comment, you can "easily" solve your problem with a few "simple" operations.

private static final String DICTIONARY = "0123456789"
       + "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
       + "abcdefghijklmnopqrstuvwxzy";
private static final int MaxNumber = 238328;
private static final int base = DICTIONARY.length();

public static String generateCode(int num) {
    if (num < 1 || num > MaxNumber) {
        throw new IllegalArgumentException("Illegal input value: " + num);
    }

    int value = num - 1;

    char ac = DICTIONARY.charAt(((value / base) / base) % base);
    char bc = DICTIONARY.charAt((value / base) % base);
    char cc = DICTIONARY.charAt(value % base);
    return new String(new char[]{ac, bc, cc});
}

Note that I compute each digit separately (in to ac, bc, and cc) and then return the combination as a string.

Further, when running the code, I discovered that you are off on your zzz assertion .... the max value you propose for zzz is wrong, what you have is yyy.

See the code running on ideone: https://ideone.com/ZPS6fe

For the inputs supplied in the tests I get the values:

Value 0 Error Illegal input value: 0
Value 1: 000
Value 2: 001
Value 1000: 0G7
Value 238328: yyy
Value 238329 Error Illegal input value: 238329