Reverse the digits of an Integer

Question

Reverse digits of an integer.

Example1:

x = 123,

return 321 Example2:

x = -123,

return -321

Return 0 if the result overflows and does not fit in a 32 bit signed integer

This is my approach:

public class Solution {
    public int reverse(int A) {

        String ans = " ";
        String finalAns = " ";
        int range = 2147483647;
        if( (A > 0) && (A <= range))
            {
                ans = String.valueOf(A);
                return (Integer.parseInt(new StringBuilder(ans).reverse().toString()));
            }
        if( (Math.abs(A) > range))
            return 0;

        else if ( (A > - (range + 1)) && (A < 0) )
            {
                ans = String.valueOf(Math.abs(A));

                if( Math.abs(A) > range)
                    return 0;

                finalAns =  (new StringBuilder(ans).reverse().toString());

                double d = Double.parseDouble(finalAns);
                if( d  == (int)d )
                    return Integer.parseInt( "-" + finalAns);
                else
                    return 0;
            }
        else
            return 0;
    }
}

I have following questions regarding my code:

1) Is there a better approach to solve this question?

2) Are there any Java coding conventions that I have violated gravely?

3) Am I doing redundant steps?

Reference

Answer 1

If I were presented with that solution as an answer to an interview question, I would definitely question your judgement and taste, even if the solution worked correctly — which it doesn't.

The value for range, 2147483647, is a magic number, better written as Integer.MAX_VALUE. However, even if (A > 0) && (A <= range) is true, the reversed result might not fit in an int, in which case Integer.parseInt(…) would throw a NumberFormatException.

I have previously advised you to avoid using floating-point operations when dealing with integer arithmetic. If your goal is to handle int overflow, then use long.

In any case, using StringBuilder.reverse() is, in my opinion, a bad idea. CPUs are very efficient at arithmetic, but less adept at handling strings. Both String.valueOf(int) and Integer.parseInt(String) do a lot of work behind the scenes. Though the inefficiency is quite tolerable for all practical purposes, I would consider the solution to have missed the mark, if judged as an interview solution.

I'd be happier with a purely arithmetic solution like this, which should be far more efficient, and that demonstrates your ability to write a loop:

public int reverse(int a) {
    long reversed = 0;
    while (a != 0) {
        reversed = 10 * reversed + (a % 10);
        a /= 10;
    }
    return (reversed == (int)reversed) ? (int)reversed
                                       : 0;    // int overflow detected
}

Note that the overflow detection is also much simpler. There are no special cases — we simply check whether downcasting from long to int can be safely done. Also note that (a % 10) produces a negative result when a is negative, so that no special handling is needed for negative numbers either.

Answer 2

Here are my comments, in order of severity:

Bugs

Did you test your solution? because it does not work properly. Try to invoke the method with argument 2147483646 (max integer - 1)

Performance

In the place where you do check for overflow, you first parse into double, and when you decide it is safe, you parse again into int. you could just return the double casted into an int.

Code Clarity

1. You use redundant parenthesis in several places:
   for example: if( (Math.abs(A) > range))
   and also in several assignments like
   finalAns = (new StringBuilder(ans).reverse().toString());
   all of these do not contribute to the correctness nor clarity of the code.
   regarding usage of parenthesis for operands of && operator, personally I think this is another case of redundancy, but there are other who argue that this is ok.

2. Java has constants to define min/max values for number types. They are defined in the number wrapper classes, like Integer.MAX_VALUE

Naming Convention

1. naming convention for Java variables is camel-case, meaning they should start with lowercase letter (I am referring to the argument)

Answer 3

• The assignments of " " to the String variables ans and finalAns are pointless, because the assigned values are never used in the code before they are overwritten, so you might as well just declare the variables without assigning them a value. The issue of where to declare local variables has already been addressed in Timothy Truckle's answer.
• It is not necessary to assign the variable ans separately in the two cases of A being positive and A being negative. You can just assign String.valueOf(Math.abs(A)) to it initially, which will cover both cases in one go. Note that this will not work for Integer.MIN_VALUE, because the additive reverse of Integer.MIN_VALUE cannot be represented as an integer, but then, there are other, fundamental problems with your code, so I would view the exceptional case of Integer.MIN_VALUE as negligible at this stage of reviewing the code.

• In this line:

  if( (A > 0) && (A <= range))
  

  The check whether A <= range is pointless because A is already defined as an int, which means that it is by definition within the bounds of an integer and, in turn, less than or equal to Integer.MAX_VALUE (which is the value of range). It's as if you're doing something like this:

  void ridiculousMethod(String input) { //input is a variable of type String
      if (!(input instanceof String)) { //WTF?
          throw new IllegalArgumentException("WTF?");
      }
  }
  

  In fact, this makes even more sense than what you are doing, because in ridiculousMethod(String), the exception could actually be thrown if input is null, but in your case, A is a primitive type, which cannot be null, so there is no way that A <= range can return false.

• Next, this line:

  return (Integer.parseInt(new StringBuilder(ans).reverse().toString()));
  

  Here, Integer.parseInt(String) will throw a NumberFormatException if the reverse of the integer cannot be represented as an integer. I already pointed this out in a comment to your question, but I don't think you understood what I meant, because in your response, you simply did not acknowledge the fact that this part of your code is broken, so I will illustrate the problem step by step:

  Let's take, for example, the input 1000000009. This number can be represented as an integer. The reverse of this number is 9000000001, which can not be represented as an integer because it is too large. So what happens if you pass 1000000009 to your method? We enter the first if block and ans is set to the String "1000000009". Next, a StringBuilder is constructed from ans and then reversed, so this StringBuilder now represents the character sequence "9000000001". This StringBuilder is then converted back to a String which is passed to Integer.parseInt(String), and now it's game over. Integer.parseInt(String) detects that the String "9000000001" does not contain a number that can be represented as an integer, so it does what its documentation states it does in such cases: throw a NumberFormatException. However, the requirement was that, if the reverse of the input cannot be represented as an integer, the program should return 0. Hence, your code is broken.

• if( (Math.abs(A) > range))
  

  This will never happen. The only case where the actual absolute value of A would exceed Integer.MAX_VALUE is if A is Integer.MIN_VALUE. But Math.abs(Integer.MIN_VALUE) evaluates to Integer.MIN_VALUE, because the actual absolute value of Integer.MIN_VALUE can, as I already pointed out earlier, not be represented as an int.

  In fact, the pointlessness of the above line can be explained even more simply: Math.abs(int) returns an int and, as I have already explained, an int cannot exceed the maximum value of an int.

• else if ( (A > - (range + 1)) && (A < 0) )
  

  The expression - (range + 1) does not do what you seem to think it does. It does, ironically, evaluate to Integer.MIN_VALUE in the end, which is probably what you want, but it does not get there the way you'd expect if you simply saw this expression outside the context of programming and maximum/minimum values of variables. The problem is that range + 1 does not evaluate to 2147483648, but to -2147483648, which is equivalent to Integer.MIN_VALUE, because range is an int, so 1 is interpreted as an int too, and adding two ints will always result in an int (in this case, an overflow occurs in the addition, which is why the result is not the real result of the addition). Note that, if you were to write (long) range + 1 or range + 1L, i.e. if you make one of the two summands a long instead of an int, then the other summand will also be implicitly converted to a long, and the result will be indeed 2147483648 because adding two longs will result in a long, and with longs, no overflow occurs.

  So range + 1 evaluates to Integer.MIN_VALUE, and, as I have already explained earlier, the additive inverse of Integer.MIN_VALUE will also evaluate to Integer.MIN_VALUE when represented as an int.

My suggestion would be to rework your code and request another review, since correcting some of the issues mentioned above could possibly require radical alterations to the code (it is perfectly acceptable to post a new question with a reworked version of a code from another question, but if you do, you should mutually link the questions).

Answer 4

Thanks for sharing your code.

1) Is there a better approach to solve this question?

You could use regular expressions like this:

public class Solution {
    public int reverse(int A) {
      Matcher signAndNumber = 
            Pattern.compile("(-)?(\\d+)")
                   .matcher(Integer.toString(A));
      if(signAndNumber.find()) {
        try{
          return Integer.parseInt(
                    signAndNumber.group(1)
                    + new StringBuilder(signAndNumber.group(2))
                         .reverse()
                         .toString());
        } catch (Exception e){
          // ignore and fall through
        }
      }
      return 0;
   }
}

---

2) Are there any Java coding conventions that I have violated gravely?

Your code looks quite good from that point of view. I's see only a few flaws:

1. Placement of the curly braces {. be consequent with that.

   As long as you don't collaborate with others it doesn't really matter if you place them at the end of the preceding statement they belong to or on a line of there own.

   When joining a team you should negotiate that with the others so that it is consistent throughout the project. You might get used to use the auto formatter of your IDE to take care of that.

2. Declare local variables as late as possible.

   You declare some local variables at the beginning of your method. You use them in different parts of your method. but these parts are somewhat unrelated since each part ends with a return statement. Therefore any value assigned in one part has no meaning it any of the other parts.

   If you would declare these variables (multiple times) immediately before first use it would help your IDEs automated refactoring extract method to create methods from that parts:

   With your version the refactoring would have an unneeded parameter like this:

   private int positiveAnswer(int A, String ans){   
       ans = String.valueOf(A);
       return (Integer.parseInt(new     StringBuilder(ans).reverse().toString()));
   }
   
   public int reverse(int A) {
       String ans = " ";
       String finalAns = " ";
       int range = 2147483647;
       if( (A > 0) && (A <= range))
       {
            return positiveAnswer(A, ans);
       }
   // ...
   

   The version with "late declaration" would come out as

   private int positiveAnswer(int A){   
       String ans = String.valueOf(A);
       return (Integer.parseInt(new StringBuilder(ans).reverse().toString()));
   }
   
   public int reverse(int A) {
       int range = 2147483647;
       if( (A > 0) && (A <= range))
       {
           return positiveAnswer(A);
       }
   // ...