5
\$\begingroup\$

I have written a Java program that converts an integer to String digit by digit and concatenate them together using the + operator without touching the Stock Java API library.

I like to have feedback on my code. Where do I need to improve. If I need to deduct something. So please criticize me. Thank you.

import java.util.Scanner;

public class StrFromInteger {

    /*
     * a single digit is passed as an argument
     * And a matching digit of String type
     * is returned.
     */
    public static String returnDigitString(int digit) {
        String res = "";

        switch(digit) {
            case 0:
                res = "0"; break;

            case 1:
                res = "1"; break;

            case 2:
                res = "2"; break;

            case 3:
                res = "3"; break;

            case 4:
                res = "4"; break;

            case 5:
                res = "5"; break;

            case 6:
                res = "6"; break;

            case 7:
                res = "7"; break;

            case 8:
                res = "8"; break;

            case 9:
                res = "9"; break;
        }
        return res;
    }

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        //Scan the integer as int
        Scanner scn = new Scanner(System.in);
        int number = scn.nextInt();

        //find the number of digits using logarithm
        //if input number is not equal to zero because
        //log of zero is undefined otherwise if input
        // number zero length is equal to 1
        int input = number;
        int length = 0;
        if(number != 0) {
        length = ( int ) (Math.log10(number) + 1 );}
        else if(number ==0) {
            length = 1;
        }

        //Save each digit in String format by passing
        // the integer digit to the returnDigitString()
        //method one by one
        String[] reverseStr = new String[length];


        String digits = "0123456789";

        int remainder =0;
        int result = number ;
        --length;
        number = length;
        String strSeq = "";
        String valStr = "";

        // loop through the whole integer digit by digit
        //use modulo operator get the remainder
        //save it in remainder. then concatenate valStr 
        //returned from returnDigitString()
        //method with previous String of Digits. Divide the result by 10. Again 
        //repeat the same process. this time the modulo and the
        //number to be divided will be one digit less at each decremental 
        //iteration of the loop.
        for(int i = number; i >= 0; --i) {

            remainder = result % 10;

            valStr = returnDigitString(remainder);
            strSeq = valStr + strSeq;

            result = result / 10;
        }

        //Print the string version of the integer
        System.out.println("The String conversion of " + input + " is: " + strSeq);
    }

}
\$\endgroup\$
7
\$\begingroup\$

Things I like about your code

  • The idea to calculate the length of a number with the logarithm is really good!
  • In my opinion you are writing good comments.
  • Good variable names
  • Works as intended, without (to my knowledge) any bugs.

Criticism

returnDigitString()

  • It is considered bad practice to put more than one command into one line. So please make line breaks after every ";".
  • Your solution is pretty long (over 30 lines) in comparison to the complexity of the problem. You could also have done something like that:
    public static String returnDigitString(int digit) {
        String res = "";
        String[] digits = {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
        for(int i = 0; i <= 9; i++) {
            if(digit == i) {
                res += digits[i];
                break;
            }
        }
        return res;
    }

main()

  • You are not using the array "reverseStr". The String "digits" is not used either.
  • When I started your program the first time, I didn't know what to do, because your program didn't tell me. Before scanning a user input, I would tell the user to input something.
System.out.println("Please enter number:");
Scanner scn = new Scanner(System.in);
int number = scn.nextInt();

If you want to improve this point even further (which is highly recommended!), you can use something like that (you will have to use import java.util.InputMismatchException;):

System.out.println("Please enter number:");
Scanner scn = new Scanner(System.in);
int number;
while(true) {
    try {
        number = scn.nextInt();
        break;
    }
    catch(InputMismatchException e) {
        System.out.println("That's not a number!");
        scn.nextLine();
    }
}

This will check, whether the user really enters a number. If the user enters something else, the program will ask him again to enter a number.

  • Something like that is considered bad practice:
if(number != 0) {
length = ( int ) (Math.log10(number) + 1 );}

Please write

if(number != 0) {
        length = (int) (Math.log10(number) + 1);
}

instead.

  • "valStr" is not necessary. You can just write:
strSeq = returnDigitString(remainder) + strSeq;

But this really is a minor point and just my personal opinion. It's fine to use an extra variable for this.

Codestructure

  • I would use an extra method for the content of the main-method. Just use the main-method to call the new method.
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Couldn't the third line of your returnDigitString have been return digits[digit]? \$\endgroup\$ – Stobor May 1 at 9:20
  • 2
    \$\begingroup\$ Yes, that's true, but I also wanted to provide an alternative structure to the switch-case-statement. \$\endgroup\$ – Philipp Wilhelm May 1 at 9:28
  • 1
    \$\begingroup\$ I'm not so familiar with Java in particular, but the code for(int i = 0; i <= 9; i++) { if(digit == i) { ... } } seems like a bad suggestion - it is equivalent to if(0 <= digit && digit <= 9){ ... } replacing each instance of i with digit in ... - which is far more clear (and more efficient, if we care). It seems like this suggestion gives the wrong impression - if you need to use switch, use it. If your code is just dealing with data, look up the data as digits[digit]. I don't see any use case for a for loop which turns out to be equivalent to an if statement. \$\endgroup\$ – Milo Brandt May 1 at 15:49
  • \$\begingroup\$ @Milo Brandt, you are abolutely right, but I wanted to avoid long code for a small problem. Of course the solution suggested by Stobor is the best idea. \$\endgroup\$ – Philipp Wilhelm May 1 at 16:30
  • \$\begingroup\$ Calculating a logarithm is still a somewhat heavy-hitting exercise. It will also cause this code to break spectacularly when the input number is negative. \$\endgroup\$ – Eric May 1 at 20:41
9
\$\begingroup\$

Personally I think your algorithm has been made a lot more complex than needed.

Is the concatenation a requirement? If not, you can simplify by directly converting each digit into a char and storing it in a char[]. This way instead of inefficiently concatenating each digit onto the string, you can use the string constructor overload that takes a char[].

With this simplification, the method can be reduced to just a few lines:

  public static String intToString(int num) {
    if(num == 0){
      return "0";
    }
    int count = 0;
    boolean isNeg = false;
    if (num < 0) {
      num *= -1;
      count = 1;
      isNeg = true;
    }
    count += (int) Math.log10(num) + 1;
    char[] digits = new char[count];
    if (isNeg) {
      digits[0] = '-';
    }
    --count;
    while(num > 0) {
      digits[count--] = (char) ((num % 10) + '0');
      num /= 10;
    }
    return new String(digits);
  }
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Might calling Math.log10() have a significant overhead (compared to using a StringBuilder instead of the char array)? \$\endgroup\$ – gidds May 1 at 16:52
  • \$\begingroup\$ To use a stringbuilder you would have to insert each character at the start or reverse the string. Either way I don't think log10 would be worse than that. \$\endgroup\$ – tinstaafl May 2 at 0:55
7
\$\begingroup\$
public class StrFromInteger {

This is a convertor, so I would expect some kind of actor in the name, say DecimalStringCreator. What you've currently got is more like a method name.


public static String returnDigitString(int digit) {

The comment before this function is almost a JavaDoc. Generally public methods should be documented with the JavaDoc within /** and */.

That something is returned should be logical, try digitToString. As the string is always one character, a digitToCharacter might be better. Unless you want to make it part of programming interface, this method should probably be private.

Note that an integer might not just be a digit. I would call it digitValue instead and then add a guard statement, such as:

if (i < 0 || i > 9) {
   throw new IllegalArgumentException("Value of digit not in the range [0..9]");
}

or something similar.


String res = "";

Assigning an immutable empty string is almost never a good idea. Don't assign values unless you really have to.


switch(digit) { ... }

Whenever possible, try and not start calculating yourself. Let the computer handle it. In this case it is important to know that the numbers are all situated in character range 0x0030 for the zero and 0x0039 for the 9 - in order of course. The location is not that important, but the order is, as it allows you do to

char digit = '0' + i;

instead.

In Java it is perfectly valid to use return "3"; by the way. That way you would not need the many break; statements. Generally we put break on a separate line by the way.


// TODO Auto-generated method stub

Always remove those kind of comments before posting or - for that matter - checking into source control (e.g. Git).


public static void main(String[] args) {

A main method is fine for setting up a Scanner, retrieving user input and producing output. But the actual conversion from int to String should be in a separate method.


//find the number of digits using logarithm

Whenever you type this kind of comment, you should create a method. In this case calculateNumberOfDigits() would be a good name. Now that's clear, you can actually remove the comment - so you would not have to do all that much.


int input = number;

First of all, the scanner produces the input. You only need one variable for this because neither number or input is ever changed.


int length = 0;

Another assignment that isn't needed. Java will complain if variables are not assigned. This is useful to find bugs as well, so if the variable is always assigned then specifying a default value is not needed.

if(number != 0) {
length = ( int ) (Math.log10(number) + 1 );}
else if(number ==0) {
    length = 1;
}

Oy, bad indentation and bad usage of white space. This should be:

if(number != 0) {
    length = (int) (Math.log10(number) + 1);
} else if(number == 0) {
    length = 1;
}

String[] reverseStr = new String[length];

String arrays are generally not a good idea. In this case you can always simply perform String concatenation using +. Note that it is completely possible to add Strings / characters at the start of a string as well.


--length;

Generally we use length--. Don't use --length, unless you need to use the original length value within a larger expression. If possible simply use length-- afterwards: expressions without so called side effects are much easier to understand.


number = length;

Do not reassign variables to other values than that they originally hold. If the meaning of a variable changes then you can be sure that confusion will arise.


The main idea of getting a list of digits is OK:

for(int i = number; i >= 0; --i) {

    remainder = result % 10;

    valStr = returnDigitString(remainder);
    strSeq = valStr + strSeq;

    result = result / 10;
}

But beware of variable naming. result is not really the result that you are looking for; that's the string after all. So another name should be preferred, e.g. numberValueLeft or just valueLeft.

Note that if valueLeft is zero then the calculation is finished, so that's another way of determining the end of the calculation.


Here's my take:

/**
 * Creates a decimal String for a value that is positive or zero.
 * 
 * @param value the value to convert
 * @return the string representing the value
 */
public static String toDecimalString(int value) {
    // guard statement
    if (value < 0) {
        throw new IllegalArgumentException("Negative numbers cannot be converted to string by this function");
    }

    if (value == 0) {
        return "0";
    }

    String decimalString = "";

    int left = value;
    while (left > 0) {
        int digitValue = left % 10;
        char digit = (char) ('0' + digitValue);
        decimalString = digit + decimalString;
        left = left / 10;
    }

    return decimalString;
}

Note that you would normally use StringBuilder for this kind of thing, but I presume that that's not allowed in this case.

I always indicate what kind of string is being returned with a function. I've seen tons of somethingToString() functions that are absolutely unclear of what is being returned. Now I think that a decimal string is what most people expect, but I've also seen somethingToString() functions that return hexadecimals, base 64 or whatnot, so making it clear helps the reader.

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

Your algorithm only works for values which are zero or positive. Java's "int" type is signed, so you need to consider negative numbers too. Your algorithm will fail hard on this, not least because taking a log of a negative number returns NaN, which results in zero when you cast it to int.

Your first step in the algorithm should be to handle the sign of the number. After that you can sort out how to process an unsigned value.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Here is my updated code. After this any comments will be highly appreciated.

package IntegerToString;
import java.util.Scanner;


/**
 * @author      Adrian D'Costa 
 * @version     1.1                (current version number of program)
 * @since       1.0          (the version of the package this class was first added to)
 */

public class StrFromInteger {

    /* *
     * a single digit is passed as an argument And a matching digit of String
     * type is returned. 
     * 
     * @param digit a digit of the whole integer
     * 
     * @return      return a String representation of the digit
     */
    public static String returnDigitString(int digit) {
        String res = "";

        String[] digits = { "0", "1", "2", "3", "4", "5", "6", "7", "8", "9" };
        for (int i = 0; i <= 9; i++) {
            if (digit == i) {
                res += digits[i];
                break;
            }
        }
        return res;

    }
    /* *
     * Take the input number, if it is less than zero multipy it by -1.
     * loop through the whole integer digit by digit
     * use modulo operator get the remainder
     * save it in remainder. then concatenate valStr
     * returned from returnDigitString()
     * method with previous String of Digits. Divide the result by 10. Again
     * repeat the same process. this time the modulo and the
     * number to be divided will be one digit less at each decremental
     * iteration of the loop. Then print the String and if it is less than zero
     * concatenate "-" at the beginning of total String representation of int
     * otherwise just print the String representation of int.
     * 
     * @param length number of digits in the integer
     * @param number the integer number itself
     * @param isPosite is positive or not
     */
    public static void printInt(int length, int number, boolean isPositive ) {
        int input = number;

        int remainder = 0;
        int result = (number < 0 ? -1 * number : number);
        --length;
        number = length;
        String strSeq = "";
        String valStr = "";

        // loop through the whole integer digit by digit
        // use modulo operator get the remainder
        // save it in remainder. then concatenate valStr
        // returned from returnDigitString()
        // method with previous String of Digits. Divide the result by 10. Again
        // repeat the same process. this time the modulo and the
        // number to be divided will be one digit less at each decremental
        // iteration of the loop.
        for (int i = number; i >= 0; --i) {

            remainder = result % 10;

            valStr = returnDigitString(remainder);
            strSeq = valStr + strSeq;

            result = result / 10;
        }
        if (!isPositive) {
            strSeq = "-" + strSeq;
        }
        // Print the string version of the integer
        System.out.println("The String conversion of " + input + " is: " + strSeq);
    }

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        // Scan the integer as int
        Scanner scn = new Scanner(System.in);
        int number = scn.nextInt();

        // find the number of digits using logarithm
        // if input number is not equal to zero because
        // divide the input by 10 each number it will be
        // reduced by 1 digit and increment the length
        int input = number;
        int length = 0;
        if (number != 0) {
            int num = number;

            while (num != 0) {
                // num = num/10
                num /= 10;
                ++length;

            }
        } else if (number == 0) {
            length = 1;
        }
        printInt(length, input, (input < 0 ? false : true));
    }

}
| improve this answer | |
\$\endgroup\$

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.