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));
    }  
}
Improve this question
\$\endgroup\$
1
  • \$\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, 2013 at 17:08

2 Answers 2

Reset to default
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\$
7
  • \$\begingroup\$ Is there a + missing in your initialization of DigitOne? Also shouldn't static finals be UPPER_SNAKE_CASE? \$\endgroup\$
    – Bobby
    Oct 2, 2013 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, 2013 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, 2013 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, 2013 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, 2013 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\$
6
  • \$\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, 2013 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, 2013 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, 2013 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, 2013 at 16:54
  • \$\begingroup\$ Was the point of this to maximize speed? That wasn't stated in the question. \$\endgroup\$
    – WeaponsGrade
    Oct 2, 2013 at 17:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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