I am attempting to come up with a better method for doing this:
public static String longToPlayerName(long name) {
int i = 0;
char[] nameCharacters = new char[12];
while (name != 0L) {
long ll = name;
name /= 37L;
nameCharacters[11 - i++] = playerNameXlateTable[(int) (ll - (name * 37L))];
}
return new String(nameCharacters, 12 - i, i);
}
public static long playerNameToInt64(String s) {
long l = 0L;
for (int i = 0; (i < s.length()) && (i < 12); i++) {
final char c = s.charAt(i);
l *= 37L;
if ((c >= 'A') && (c <= 'Z')) {
l += (1 + c) - 65;
} else if ((c >= 'a') && (c <= 'z')) {
l += (1 + c) - 97;
} else if ((c >= '0') && (c <= '9')) {
l += (27 + c) - 48;
}
}
while (((l % 37L) == 0L) && (l != 0L)) {
l /= 37L;
}
return l;
}
I can't seem to come up with a better way. Any ideas would be excellent!
3 Answers 3
Pretty good overall, but a few things:
- Don't hard-code the 37. Instead, use the translation table's size.
ll
is a bad variable name. It's not descriptive at all, and it looks like11
.Your loop in
longToPlayerName
can be simplified:final int base = playerNameXlateTable.length; while (name != 0L) { final long ch = name % base; nameCharacters[11 - i++] = playerNameXlateTable[ch]; name /= base; }
Be careful with inlined
++i
ori++
. They can be easy to overlook, and the human brain (mine anyway) doesn't parse arithmetic expressions that cause mutations very well.I think you've overused parenthesis a bit. For example, I would just do:
if (c >= 'A' && c <= 'Z') {
playerNameToInt64
should beplayerNameToLong
s
is a bad parameter name. Call it something likename
- Other than for loop controls, single letter variables should typically be avoided.
- It should also probably be
final
, but it's immutable, so it doesn't really matter in this case.
- Use
Character.toLowerCase
to eliminate one of the branches inplayerNameToInt64
. - It might be better to group your shifts like
1 + (c - 97)
instead of how they're grouped. It doesn't matter in the current code, but if(1 + c) - 97
can in theory overflow, but1 + (c - 97)
cannot Rather than 97/65/48, use character literals:
1 + (c - 'a')
The truncation loop is a bit troubling. For a-zA-Z0-9, there never will be any trailing 0 valued base-37 digit. This means that you're worried an invalid character will be provided. But... what if one is in the middle of the string?
- If you made a reverse translation table, you could decouple your code from ASCII
- no point in doing this though, as it's probably always going to be ASCII (or a superset of ASCII)
- Fold the double comparison in the
toLong
loop into aMath.min()
call and use that - Assuming this class is dedicated to player name translation and only player name translation (as it should be),
playerNameXlateTable
could be shortened totranslationTable
- In the context of a single-responsibility class, it's obvious what the table is translating
- Though partly I'm just saying this because
xlate
makes me cry, butplayerNameTranslationTable
does too in all of its 26 character glory.
- Will you ever have any other name encodings? If so, it might be worth-while to consider making an interface and having these methods be non-static.
- It would make your code a bit more verbose, but it would allow for a bit more decoupling
- Make 12 a constant
- It would be nice to calculate it since
maxLetters = floor(log(2^LONG_BITS) / log(37))
, but there's no non-hideous way to calculate this - If it's a constant and you later expand the translation table, you'll only have to change 12 in one place instead of multiple. It's always good to reduce the chance of introducing future bugs.
- It would be nice to calculate it since
Since the code is short, I gave it a go at revising it:
public class Review {
private static char[] translationTable = buildTranslationTable();
private static int maxLetters = 12;
public static void main(String[] args)
{
System.out.println(playerNameToLong("Corbin92"));
System.out.println(longToPlayerName(324533943486L));
}
public static String longToPlayerName(long name) {
int i = 0;
char[] nameCharacters = new char[maxLetters];
final long base = translationTable.length;
while (name != 0L) {
final long ch = name % base;
nameCharacters[maxLetters - ++i] = translationTable[(int) ch];
name /= base;
}
return new String(nameCharacters, maxLetters - i, i);
}
public static long playerNameToLong(final String name) {
final long base = translationTable.length;
final int len = Math.min(name.length(), maxLetters);
long l = 0L;
for (int i = 0; i < len; ++i) {
final char c = Character.toLowerCase(name.charAt(i));
l *= base;
if (c >= 'a' && c <= 'z') {
l += 1 + (c - 'a');
} else if (c >= '0' && c <= '9') {
l += 27 + (c - '0');
}
}
return l;
}
private static char[] buildTranslationTable()
{
char[] table = new char[37];
for (char c = 'a'; c <= 'z'; ++c) {
table[1 + c - 'a'] = c;
}
for (int i = 0; i < 10; ++i) {
table[i + 27] = (char) (i + '0');
}
return table;
}
}
-
\$\begingroup\$ Wow! Thanks for the excellent tips! :- ) \$\endgroup\$johnathenwhiter94– johnathenwhiter942013年07月10日 02:53:06 +00:00Commented Jul 10, 2013 at 2:53
-
\$\begingroup\$ @AtomicInt_ Glad I could help! If possible at all, by the way, you should try to use abuzittin gillifirca's approach. It uses a different encoding with different properties, so it might not be possible, but what your code is essentially changing bases, which his done in 2 lines :). \$\endgroup\$Corbin– Corbin2013年07月11日 18:44:34 +00:00Commented Jul 11, 2013 at 18:44
You can directly convert an alphanumeric string to int64 and back in Java, using Long.parseLong
and Long.toString
using base 36. Like so:
System.out.println(Long.toString(994264677686L, 36));
System.out.println(Long.parseLong("Corbin92", 36));
What you then need to do is validation, for example check that the input string consists of alphanumeric characters; and handle edge cases, for example check that string is not too long or the number is not too small or too big etc.
-
\$\begingroup\$ Hmmm, well that's awkward.... Seems my Java rustiness is showing :). \$\endgroup\$Corbin– Corbin2013年07月09日 08:59:47 +00:00Commented Jul 9, 2013 at 8:59
Note: I realize both that this is old and that there is a more direct answer. But I think that knowing how and when to use a StringBuilder
is more important in general than solving this problem.
public static String longToPlayerName(long name) { int i = 0; char[] nameCharacters = new char[12]; while (name != 0L) { long ll = name; name /= 37L; nameCharacters[11 - i++] = playerNameXlateTable[(int) (ll - (name * 37L))]; } return new String(nameCharacters, 12 - i, i); }
This is what a StringBuilder
is designed to do. Then you don't have to muck with internals like i
. Consider
private static final char[] ALPHABET = " abcdefghijklmnopqrstuvwxyz0123456789".toCharArray();
public static String longToPlayerName(long packed) {
StringBuilder reversed = new StringBuilder();
for (; packed > 0L; packed /= ALPHABET.length) {
reversed.append(ALPHABET[packed % ALPHABET.length]);
}
return reversed.reverse().toString();
}
No i
to maintain. And it doesn't have to jump through hoops to get just the part of the array that represents the characters.
Doesn't have to hard code the length. Nor the size of the alphabet.
It's more obvious that the intermediate representation is reversed.
While I love math, each math operation is a potential error. This reduces the number of math operations to three (including the comparison). The original code had six (comparison, subtraction, increment, multiplication, subtraction, subtraction) and two hard coded constants.
And again, the base 36 solution is more appropriate to this particular problem. It's even a more compact encoding. But if you are writing a solution to a problem like this, a StringBuilder
is a better tool than a character array.