6
\$\begingroup\$

I was reading a Java book and I came up with this kind of problem and I tried to solve it:

Problem: Write an application that inputs one number consisting of five digits from the user, separates the number into its individual digits and prints the digits separated from one another by three spaces each. For example,

Input:

42339

Output:

4   2   3   3   9

Solution:

package practice;

import java.io.IOException;
import static java.lang.System.in;
import java.util.InputMismatchException;
import java.util.Scanner;

public class practice {

    public static void main(String[] args) throws IOException {                      
        try(Scanner input = new Scanner(System.in)){

            System.out.print("");
                int a = input.nextInt();

            char[] newString = Integer.toString(a).trim().toCharArray();

            if (newString.length < 5 || newString.length > 5) {

                System.out.println("Input should be exactly 5.");
                return;

            } else {

                for (int i = 0; i < newString.length; i++) {
                    System.out.print(newString[i] + "   ");
                }
            }

            System.out.println();

         } catch (InputMismatchException e){

             System.out.println("Error: Invalid input.");

        } finally {

            in.close();

        }
    }  
}

My questions are:

  • Did I do something wrong?

  • Is there any way to solve this problem more efficiently?

\$\endgroup\$
  • 4
    \$\begingroup\$ Quick note: why not if (newString.length != 5)? \$\endgroup\$ – cbreezier Jun 1 '15 at 7:03
4
\$\begingroup\$

Input and validation

Using a Scanner is a good idea, and using a try-with-resources block is the right thing to do.

You don't need to close the Scanner or the underlying System.in. The whole point of the try-with-resources block is that resources will be automatically closed for you.

Note that neither the Scanner(Readable) constructor nor Scanner.nextInt() will ever throw an IOException. The only exception you need to catch (but didn't) is NoSuchElementException. (Well, also InputMismatchException, but that is just a subclass of NoSuchElementException.) Your program will crash with a NoSuchElementException if you send it an EOF at the prompt.

The error messages aren't quite right. The first message should read: "Input should be exactly 5 digits." The second message is unhelpful: you should tell the user what would be considered valid input.

The validation is not quite strict enough: a negative four-digit integer will also be accepted.

Output

Strictly speaking, the output is not quite right, in that you also append three spaces after the fifth digit. On screen, the output will look the same, but if you pipe the output to a program that examines the bytes, you'll see the trailing spaces.

You don't need to call String.trim().

newString is not a good name: it's not a string, and there is no oldString. If I had to pick a name, I'd call it digits.

Suggested solution

With Java 8, the simplest solution is to use String.join().

public static void main(String[] args) {
    try (Scanner input = new Scanner(System.in)) {
        int n = input.nextInt();
        if (!(1e5 <= n && n < 1e6)) {
            System.out.println("Input should be exactly 5 digits.");
            return;
        }
        System.out.println(String.join("   ", String.valueOf(n).split("")));
    } catch (NoSuchElementException e) {
        System.out.println("Error: No integer was given.");
    }
}
\$\endgroup\$
4
\$\begingroup\$

This exercise is pretty strange. It speaks about number and digits, but they don't matter at all.

If an input validation is required, I'd go for

 if (s.matches("\\d{5}")) ...

which test that the input contains exactly five digits. If you're interested in speed use

 private final static Pattern VALID_INPUT = Pattern.compile("\\d{5}");

 if (VALID_INPUT.matcher().matches()) ...

If you're interested in exotic digits (like U+0665 ٥ or U+07C7 ߇), use the Pattern.UNICODE_CHARACTER_CLASS flag.

Now we can definitively forget about digits.

For the transformation,

s.replaceAll("", "   ").trim()

is the simplest and pretty hacky solution (the empty string matches everywhere, which gets fixed by the trim). The saner alternatives have been already given.

\$\endgroup\$
3
\$\begingroup\$

Generally, your code looks good, with only minor issues.

The first is the finally block:

} finally {

    in.close();

}

This code will not compile, since there is no in variable to close... Using try with resources actually replaces any close statements which needs to be done on your resources, so you can safely delete this block - the stream you created will be closed for you.

Aside from that, I suggest that you read up on Java code conventions, specifically on class names, which should start with an uppercase (Practice rather than practice). Also, give meaningful names to your variables - newString does not infer what is the role of this variable...

Another thing which I noticed (and we are getting into the very small things) - your spacing (empty lines between code lines) is a bit excessive, and makes your code a little less readable. Use empty lines like you would use paragraphs in prose:

char[] newString = Integer.toString(a).trim().toCharArray();

if (newString.length < 5 || newString.length > 5) {
    System.out.println("Input should be exactly 5.");
    return;
} else {
    for (int i = 0; i < newString.length; i++) {
        System.out.print(newString[i] + "   ");
    }
}

System.out.println();

Regarding efficiency - your code has linear complexity*, which is the best complexity one should hope for in this kind of problem.

* Computing complexity is a big topic (you can see here for some extensive answers on the subject); to correctly compute it, you need to have a reasonable knowledge about the methods you use (like trim() and toCharArray()). Generally, you should look for loops in your code (you have one), and your method calls. If all the method calls outside loops have linear complexity, all of those inside linear loops are of constant complexity, and there are no non-linear loops - your complexity is linear.

You cache results (you calculate newString once rather than splitting it over and over again), you validate your input in a very elegant manner, and I can't see any unneeded excess of procedures which should be optimised out, so good job!


Update - regarding my first comment,

@200_success has alerted me to the fact that I was wrong regarding in.close() not compiling, since there is an import static java.io.System.in clause.

So, couple of issues with this:

  1. Use import static sparingly, since it makes your code less readable (as happened to me - I missed the import, so I could not understand that you are closing the system input stream...)
  2. If you used import static - be uniform in your usage - you have in your code both System.in and in, this again goes for readability, since it looks like two unrelated elements, though they are the same...
  3. Regarding the code itself - there is no need in closing the system input stream, it is a single resource, not likely to cause any resource leak problems, and closing a globally shared resource might cause problems in other parts of the code, which assume that the stream is open.
  4. That being said, even if there was a real reason to close the stream, closing the Scanner stream implicitly closes the underlying stream (which is even posed as a problem on stack overflow).

All in all, I stand by my original suggestion of deleting the finally block. I might even consider not using a try block at all, leaving the resource open (since it is a global resource).

\$\endgroup\$
  • 1
    \$\begingroup\$ The code does compile. From the static import, in is System.in. \$\endgroup\$ – 200_success Jun 1 '15 at 6:40
  • \$\begingroup\$ Thank you sir for reviewing my code. Can I ask on how to determine the complexity of a code? Like, how to know if a code is in Linear complexity. Thanks. \$\endgroup\$ – Yodism Jun 1 '15 at 7:25
  • \$\begingroup\$ To clarify: the code scales linearly with the number of digits of a. Alternatively phrased, the time required is O(log a). \$\endgroup\$ – 200_success Jun 1 '15 at 7:49
  • \$\begingroup\$ The number of digits of a is log a. \$\endgroup\$ – 200_success Jun 1 '15 at 8:05
  • \$\begingroup\$ Just using the same notation as in the code. \$\endgroup\$ – 200_success Jun 1 '15 at 8:20

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.