# Implement String's valueOf function, code review request

Ok, code reviewers, I want you to pick my code apart and give me some feedback on how I could make it better or more simple.

public final class StringValueOf {

private StringValueOf () {}

// note that int max value is 10 digits
final static int [] sizeTable = { 9, 99, 999, 9999, 99999, 999999, 9999999,
99999999, 999999999, Integer.MAX_VALUE };

private final static char[] DigitOne = {
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
};

private final static char[] DigitTens = {
'0', '0', '0', '0', '0', '0', '0', '0', '0', '0',
'1', '1', '1', '1', '1', '1', '1', '1', '1', '1',
'2', '2', '2', '2', '2', '2', '2', '2', '2', '2',
'3', '3', '3', '3', '3', '3', '3', '3', '3', '3',
'4', '4', '4', '4', '4', '4', '4', '4', '4', '4',
'5', '5', '5', '5', '5', '5', '5', '5', '5', '5',
'6', '6', '6', '6', '6', '6', '6', '6', '6', '6',
'7', '7', '7', '7', '7', '7', '7', '7', '7', '7',
'8', '8', '8', '8', '8', '8', '8', '8', '8', '8',
'9', '9', '9', '9', '9', '9', '9', '9', '9', '9',
};

private static int stringSize(int x) {
for (int i = 0; ; i++) {
if (x <= sizeTable[i]) {
return i + 1;
}
}
}

private static void getChars (char[] buf, int size, int i) {
int charPos = size - 1;

if (i < 0) {
i = -i;
}

while (i >= 10) {
int r = i % 100;
i = i / 100;

buf[charPos--]  = DigitOne[r];
buf[charPos--]  = DigitTens[r];
}

if (i > 0) {
buf[charPos--] = DigitOne[i];
}

if (charPos == 0) {
buf[charPos] = '-';
}
}

public static String valueOf(int i) {
if (i == Integer.MAX_VALUE) {
return "-2147483648";
}
int size = (i < 0) ? stringSize(-i ) + 1 : stringSize(i);
char[] buf = new char[size];

buf.toString();

getChars(buf, size, i);

/**
* There are 2 ways to convert a char into string.
* 1. buf.toString()
* 2. String(buf)
*
* but we should use String(buf) because:
* 1. Mostly buf.toString would internally call String(buf)
* 2. Integer class uses new String.
*/
return new String(buf);
}

public static void main(String[] args) {
System.out.println(valueOf(101));
System.out.println(valueOf(-2010));
}
}

• The buf.toString() doesn't do anything. Also, don't you mean to test for Integer.MIN_VALUE and return -2147483648? – WeaponsGrade Oct 2 '13 at 17:08

I would replace (just to reduce size of the source-code

private final static char[] DigitOne = {
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
};

private final static char[] DigitTens = {
'0', '0', '0', '0', '0', '0', '0', '0', '0', '0',
'1', '1', '1', '1', '1', '1', '1', '1', '1', '1',
'2', '2', '2', '2', '2', '2', '2', '2', '2', '2',
'3', '3', '3', '3', '3', '3', '3', '3', '3', '3',
'4', '4', '4', '4', '4', '4', '4', '4', '4', '4',
'5', '5', '5', '5', '5', '5', '5', '5', '5', '5',
'6', '6', '6', '6', '6', '6', '6', '6', '6', '6',
'7', '7', '7', '7', '7', '7', '7', '7', '7', '7',
'8', '8', '8', '8', '8', '8', '8', '8', '8', '8',
'9', '9', '9', '9', '9', '9', '9', '9', '9', '9',
};


by

private final static char[] DigitOne =
("0123456789"+"0123456789"+"0123456789"+"0123456789"+"0123456789" //
+"0123456789"+"0123456789"+"0123456789"+"0123456789"+"0123456789")
.toCharArray();
};

private final static char[] DigitTens = {
("0000000000"+ "1111111111"+"2222222222"+"3333333333"+"4444444444" //
+"5555555555"+"6666666666"+"7777777777"+"8888888888"+"9999999999")
.toCharArray();
};


Or even generate the constants with a static method using loops.

If you want to avoid speed loss by the loop, unroll it completely. There are only 10 possible cases for the length. With a binary-decision you can determine the length with 4 if-statements and than convert the value without any loop at all. Code would get a bit long, but also very fast.

• Is there a + missing in your initialization of DigitOne? Also shouldn't static finals be UPPER_SNAKE_CASE? – Bobby Oct 2 '13 at 13:23
• @Bobby: Thanks for the missing + hint, I fixed it. And I have not changed the names of the variables, because that is pure convention. – MrSmith42 Oct 2 '13 at 13:52
• @MrSmith42 String concat is a very expensive operation, check that section of effective java encouraging string builders instead of string concay – JavaDeveloper Oct 2 '13 at 16:04
• @JavaDeveloper Static string concatenation is now resolved at compile time. But it didn't used to be; so it's no longer expensive at runtime. – WeaponsGrade Oct 2 '13 at 17:27
• @JavaDeveloper: That ok, because the .toCharArray() method call is done only once while loading the class. – MrSmith42 Oct 6 '13 at 15:57

Since it's easy to convert integer digits to their character values ('0' = 0x30..'9' = 0x39), why not:

public static String stringValueOf(int value) {
if (value == 0) return "0";
if (value == Integer.MIN_VALUE) return "-2147483648";

final boolean negative = value < 0;
if (negative) value = -value;

final StringBuilder buf = new StringBuilder();
while (value != 0) {
int digit = value % 10;
buf.append( (char)(0x30 + digit) );
value = value / 10;
}
if (negative) buf.append('-');

return buf.reverse().toString();
}


It's easy to test this for correctness over the int range; but to do so, I'd extract the StringBuilder construction from the method into a static class variable and just call buf.setLength(0) each time.

• I have some reservations in terms of efficiency. 1. Stringbuilder is less efficient then char array of fized capacity. 2. Your while loop takes twice more iterations. Rest I do admit your code is cleaner and concise – JavaDeveloper Oct 2 '13 at 8:56
• You can change the implementation of this answer to use char[] instead of StringBuilder if you prefer it. And the more iterations should not be as bad as you may think – MrSmith42 Oct 2 '13 at 9:31
• Minor nitpick, you could directly return Integer.MIN_VALUE.ToString() instead of a hardcoded string. Would mostly improve readability. ... Or you could pull those two ifs together and return Integer.toString(value). – Bobby Oct 2 '13 at 13:24
• @Bobby The only reason I took the convenience of hard-coding the MIN_VALUE as a special case is because of two's complement format and negation; -MIN_VALUE == MIN_VALUE in 2's complement. I thought the whole point of this exercise was to re-implement Integer.toString(), so you wouldn't be allowed to call what you're re-implementing (otherwise what's the point?) – WeaponsGrade Oct 2 '13 at 16:54
• Was the point of this to maximize speed? That wasn't stated in the question. – WeaponsGrade Oct 2 '13 at 17:00