3
\$\begingroup\$

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));
    }  
}
\$\endgroup\$
  • \$\begingroup\$ The buf.toString() doesn't do anything. Also, don't you mean to test for Integer.MIN_VALUE and return -2147483648? \$\endgroup\$ – WeaponsGrade Oct 2 '13 at 17:08
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is there a + missing in your initialization of DigitOne? Also shouldn't static finals be UPPER_SNAKE_CASE? \$\endgroup\$ – Bobby Oct 2 '13 at 13:23
  • \$\begingroup\$ @Bobby: Thanks for the missing + hint, I fixed it. And I have not changed the names of the variables, because that is pure convention. \$\endgroup\$ – MrSmith42 Oct 2 '13 at 13:52
  • \$\begingroup\$ @MrSmith42 String concat is a very expensive operation, check that section of effective java encouraging string builders instead of string concay \$\endgroup\$ – JavaDeveloper Oct 2 '13 at 16:04
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ – WeaponsGrade Oct 2 '13 at 17:27
  • 1
    \$\begingroup\$ @JavaDeveloper: That ok, because the .toCharArray() method call is done only once while loading the class. \$\endgroup\$ – MrSmith42 Oct 6 '13 at 15:57
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – JavaDeveloper Oct 2 '13 at 8:56
  • \$\begingroup\$ 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 \$\endgroup\$ – MrSmith42 Oct 2 '13 at 9:31
  • \$\begingroup\$ 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). \$\endgroup\$ – Bobby Oct 2 '13 at 13:24
  • 2
    \$\begingroup\$ @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?) \$\endgroup\$ – WeaponsGrade Oct 2 '13 at 16:54
  • \$\begingroup\$ Was the point of this to maximize speed? That wasn't stated in the question. \$\endgroup\$ – WeaponsGrade Oct 2 '13 at 17:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.