5
\$\begingroup\$

Which of these three methods is better?

import java.util.* ;    
public class anycode{

  public static void print_digits3(int num){
    //this method converts integer to string and parses it
    int digit = 0 ;
    String num_str = new Integer(num).toString() ;
    int counter = 0  ;
    while(counter != num_str.length()){
       System.out.println("digit: " + num_str.charAt(counter)  + "| real integer digit: " + Integer.parseInt(num_str.substring(counter,counter+1))) ;
      counter++ ;
    }

  }

  public static void print_digits4(int num){
    //this method converts integer to array of chars and parses it
     char[]num_sequence_of_chars = new Integer(num).toString().toCharArray() ;
     for(int i = 0 ; i < num_sequence_of_chars.length ; i++){
       System.out.println("digit :" + num_sequence_of_chars[i] ) ;    
     }
  }
  public static void print_digits2(int num){
    //this is the MATH method of parsing number - take every first digit div by
    // 1000,100,10,1 and update num - num% to get the rest

    //it s clunky, hard to read
    int digit = 0 ;

    Integer res_int = new Integer(num) ;

    int num_len = res_int.toString().length();// need to know if the number is 3-digit,4-digit etc
    //so we can know the power of 10 - 10^3, 10^2, 10^1 etc
    System.out.println("len of num : " + num_len) ;
    num_len-- ;
    double base = 0 ;
    int temp ;
    while( num_len >= 0){      
      temp = num ;
      base = Math.pow(10,num_len) ;      
      num = num / (int)base ;// type recasting to int is a must, it does not work without it - no idea why
      System.out.println("num :"  + num) ;
      num = (int) temp % (int)base ;      
      num_len-- ;          
  }

  }
  public static void print_digits(int num){
    int digit = 0 ;
    while(num != 0 ){
    digit = num%10 ;
    System.out.print(digit + "\t") ;
      num = num/ 10 ;      
    }
  }

  public static void main(String args[]){
              print_digits2(7658) ;
              System.out.println("--------------------------") ;
              print_digits3(7658) ;
  }

}

How can I rewrite the print_digits2? I want a more efficient MATH method of doing - no converting to string \ char[].

\$\endgroup\$
  • 1
    \$\begingroup\$ What makes you think Integer.toString() isn't the most efficient math possible for this use case? Did you look to see how it converts a number to a string, and then try to adapt that? In particular, the static void getChars(int i, int index, char[] buf) { method will be of great interest to you. \$\endgroup\$ – corsiKa May 22 '14 at 15:20
4
\$\begingroup\$

Admonition about sloppiness

These functions all "work", but the workmanship is clearly sloppy in many ways.

  • You have four functions, all printing slightly different output. If I extend main() to call all four functions…

    public static void main(String args[]){
        print_digits(7658) ;
        System.out.println("--------------------------") ;
        print_digits2(7658) ;
        System.out.println("--------------------------") ;
        print_digits3(7658) ;
        System.out.println("--------------------------") ;
        print_digits4(7658) ;
    }
    

    The output is:

    8  5   6   7   --------------------------
    len of num : 4
    num :7
    num :6
    num :5
    num :8
    --------------------------
    digit: 7| real integer digit: 7
    digit: 6| real integer digit: 6
    digit: 5| real integer digit: 5
    digit: 8| real integer digit: 8
    --------------------------
    digit :7
    digit :6
    digit :5
    digit :8
    

    The title of your question says you want to review "3 methods" — I assume you mean print_digits2(), print_digits3(), and print_digits4(), since they all print the most-significant digit first. It would have been nice to see all three functions print exactly the same output, and print_digits() print the same output in reverse. When the functionality isn't identical, it's hard to run a fair performance benchmark, for example.

  • The functions appear in an unexpected order: print_digits3(), print_digits4(), print_digits2(), print_digits(). Logical code organization would help other programmers navigate the codebase.

  • Code indentation is inconsistent. In print_digits2(), the closing brace of the while loop is misaligned. In print_digits3(), System.out.println() has an extra leading space. In print_digits(), the first two lines of the loop body are indented at the wrong level. In main(), everything is five levels too deep.

  • Furthermore, two spaces of indentation per level is non-standard and insufficient for readability. Four spaces is the norm.

  • Your naming violates capitalization conventions. The class should be capitalized as AnyCode, but would be better renamed DigitPrinterTest. Each function should be named like printDigits(). Variables such as num_str should be numStr instead.

  • The unnecessary import java.util.* should be removed.

print_digits3()

  • int digit = 0 is superfluous, and should be removed.
  • new Integer(num).toString() unnecessarily creates an Integer object. String.valueOf(num) or Integer.toString(num) would be better.
  • You have a classic initialize–test–increment loop, which should be written as a for loop for compactness and recognizability.
  • I don't understand why you do Integer.parseInt(num_str.substring(counter,counter+1))). Do you not trust that it produces the same result as .charAt(counter)?

Code cleanup:

public static void printDigits3(int num) {
    String numStr = Integer.toString(num);
    for (int i = 0; i < numStr.length(); i++) {
        System.out.println("digit: "  + numStr.charAt(i));
    }   
}

print_digits4()

Not too bad. I suggest renaming the array to digits — short and sweet. (Well, technically, it could also contain a negative sign.)

public static void printDigits4(int num) {
    char[] digits = Integer.toString(num).toCharArray();
    for (int i = 0; i < digits.length; i++) {
        System.out.println("digit: " + digits[i]);
    }
}

print_digits2()

  • int digit = 0 is superfluous.
  • You're still stringifying the number, which is, depending on your point of view, either cheating or silly (because you're discarding the work already done by .toString()). Assuming that num is positive, the length is 1 + (int)Math.log10(num).
  • Since base and temp are only used inside the loop, they should be declared inside the loop.
  • You might as well make base an int instead of a double. Promotion from int to double is done automatically as necessary (JLS Sec 5.1.2: Widening Primitive Conversion). However, when you want to treat a double as an int, you need an explicit cast to tell the compiler that you know what you're doing, since doubles can represent numbers that int cannot.

First rewrite:

public static void printDigits2(int num) {
    for (int place = (int)Math.log10(num); place >= 0; place--) {
        int temp = num;
        int base = (int)Math.pow(10, place);
        num /= base;
        System.out.println("digit: " + num);
        num = temp % base;
    }
}

If you stare at that for a while, you'll find that you can do away with temp.

public static void printDigits2(int num) {
    for (int place = (int)Math.log10(num); place >= 0; place--) {
        int base = (int)Math.pow(10, place);
        System.out.println("digit: " + num / base);
        num %= base;
    }
}

This function fails for nonpositive numbers (as did your print_digits2()), so you should add either validation, JavaDoc to declare that limitation, or code to handle it gracefully.

Another suggestion

You should consider using recursion as well, since it lets you write a very compact implementation:

public static void printDigitsRecursive(int num) {
    if (num >= 10) {
        printDigitsRecursive(num / 10);
    }
    System.out.println("digit: " + num % 10);
}

This also fails for negative inputs, so again there is some remaining work for you to do.

Performance

The recursive solution is comparable in speed to printDigits2(). (The recursive solution is faster for smaller numbers; the log-and-pow solution is faster for larger numbers.)

Both of the stringification solutions have about the same performance. The mathematical solutions beat the stringification solutions by about 30%. That makes sense, since the strings need to be allocated and garbage-collected. To be fair, stringification solves the problem with complete generality, including support for negative numbers.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you!your answer is very detailed.. i did not even think of it this way \$\endgroup\$ – ERJAN May 22 '14 at 12:25
  • \$\begingroup\$ I would expect print_digits to be the fastest, since it involves neither strings nor pow/log operations and integer division and modulo is generally quite efficient. \$\endgroup\$ – CompuChip May 22 '14 at 15:25
  • \$\begingroup\$ @CompuChip It's hardly fair to compare code that does solves a different problem, though. \$\endgroup\$ – 200_success May 22 '14 at 16:36
  • \$\begingroup\$ `printDigitsRecursive()' will print the number in the 'correct' order. Swap it round to print in reverse order as the OP appears to prefer. Also be suspicious recursive methods that allocate no local variables. They can usually (always?) be unravelled to a more efficient loop. \$\endgroup\$ – user59064 Jan 3 '15 at 0:06
6
\$\begingroup\$

To be frank, I don't really know why you need such methods to print out individual characters, I'll just make my suggestions based on what you have...

  • Your class name anycode should be in PascalCase.
  • Your method names should be in camelCase.
  • Eclipse tells me that the assignments int digit = 0; in print_digits3 and print_digits2 are not used.
  • You have this comment: // type recasting to int is a must, it does not work without it - no idea why - that's because when you divide an int with a double, the result will be double type but you are assigning that to an int value. Therefore, you are required to cast the double into an int first for the assignment to be correct.
  • Recommended to use Integer.valueOf() instead of new Integer().

Not sure what's your aversion to using the char[] way of doing the same task, but here's a suggested approach anyways for comparison (to your print_digits4 method):

private static void printDigits( int num ) {
    for ( char c : String.valueOf( num ).toCharArray() ) {
        System.out.println( c );
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The final is weird and unnecessary, in my opinion. \$\endgroup\$ – 200_success May 22 '14 at 8:40
  • \$\begingroup\$ @200_success point taken, have edited as such. :) \$\endgroup\$ – h.j.k. May 22 '14 at 8:50
5
\$\begingroup\$

The trick you are missing here is the logarithm. What you want to do is \$\log_{10}{value}\$ to get the scale.

private static void printDigits(int val) {
    int base = (int)Math.pow(10, (int)Math.log10(val));
    while (base > 0) {
        System.out.print(" " + (val / base));
        val = val % base;
        base /= 10;
    }
    System.out.println();

}

Now, your code has a number of inconsistencies and other problems.....:

  • autoboxing: The code Integer res_int = new Integer(num) ; should, in modern Java, simply be: Integer res_int = num;, and in older Java: Integer res_int = Integer.valueOf(num);
  • You keep adding a space before the line-terminating ;. This is non-standard (though, now that I see it, I, believe I may understand why.....
  • res_int.toString().length(); could be String.valueOf(num).length();, but be careful with the input value 0, or negative values.
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The loop condition should be while (base > 0), else multiples of 10s would not print the zeroes. Also, may I humbly suggest int base = (int) Math.pow( 10, Math.floor( Math.log10( val ) ) ); so that it's slightly clearer why we only want the integer value of Math.log10(val) - to get the 10^(number-of-digits) value? \$\endgroup\$ – h.j.k. May 22 '14 at 1:00
  • 1
    \$\begingroup\$ @h.j.k. - not convinced on the floor, It adds another level of abstraction on to an already complicated thing. The while (base > 0) though is .... right... and I was wrong. Oops. \$\endgroup\$ – rolfl May 22 '14 at 1:23
  • 1
    \$\begingroup\$ I'm not sure I understand why you have used 10^log(10, value)? Surely that gives you the value back? \$\endgroup\$ – J_mie6 May 22 '14 at 6:57
  • \$\begingroup\$ @J_mie6 the value here is casted aka 'rounded down' to only the integer value, so you don't get back the original value but the largest multiple of 10 that's smaller or equal to value. \$\endgroup\$ – h.j.k. May 22 '14 at 8:13
  • \$\begingroup\$ @h.j.k. ah that makes sense, clever \$\endgroup\$ – J_mie6 Jun 8 '14 at 10:39

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.