4
\$\begingroup\$

I just finished this code for my own practice purpose. This code works just fine but I understand there still a lot insidious problems. Any advice for improvement would be appreciated. So here it goes:

Package DS;

import java.util.*; 

public class basic_Array {

public static void main(String[] args) {

    int option;

    System.out.println("Welcome to basic array test, please choose your testing subject: ");

    Scanner user_option = new Scanner(System.in);


    do{

        System.out.println("Option 1: Simple one dimensional array.");
        System.out.println("Option 2: Simple two dimensional array. (under construction)");
        System.out.println("Option 0: Exit program");


        option = user_option.nextInt();
        System.out.println("current option is: " + option);

        switch(option){

        case 1:
            //simple array input method 
            simple_a1();
            break;
        case 2:
            simple_a2();
            break;
        }


    }while(option != 0);

    System.out.println("Program terminated...");
    user_option.close();

}

private static void simple_a1() {

    int[] a1 = new int[5];

    int in_num;                                                             //redundant but clear.

    Scanner user_input = new Scanner(System.in);

    for(int i = 0; i < 5; i++){

        System.out.println("Request input for item " + i);
        in_num = user_input.nextInt();
        a1[i] = in_num;
        System.out.println("Got it, next one ");
    }

    System.out.println("The numbers in a1 are: ");

    for(int i2: a1){
        System.out.print(i2 + " ");
    }

    System.out.println("\nEnd of testing 1, back to main menu...");


}

private static void simple_a2() {

    System.out.println("This function still under construction, back to main menu...");
}

}

Edit in 2/26/2017:

Thanks for EBrown and others input, I've modified my program structure and added a few new functions.

I also have a question on multiple scanners I've put in my program. I did some search on it and many people says this is also a bad practice that may confuse the programmer in long run. I think it cause the error when I trying to close the scanner object in simple_a1 method, so I only close the user_option scanner (located in main method). So my question is: will it still cause resource leak?

\$\endgroup\$
10
\$\begingroup\$

Let's talk about SRP for the moment (I'm not going to cover anything else but this). SRP (or Single-Responsibility Principle) dictates that each and every component of code should have exactly one responsibility.

So we're going to look at all your code top-to-bottom and determine what responsibilities we have in each method.

We'll start with main:

  1. Output information regarding programme definition to user;
  2. Accept and process user input;
  3. Call appropriate method(s) based on user input;

So that's more than one, but that's fine, we need to finish our analysis to make appropriate decisions on what to do next. Let's look at simple_a1:

  1. Process items in an array;
  2. Accept and process user input;
  3. Add user input to array;
  4. Print array information;
  5. Inform the caller we're going back to the main menu;

That's a lot, and even simple_a2 has more than one responsibility:

  1. Inform the user the feature is not implemented;
  2. Inform the user we are returning to the main menu;

What happens when we're not going back to the main menu again from either of those methods? Now you have multiple places that you have to modify, and in a larger, production application that would mean very possible breakages.

So we're going to eliminate responsibilities until each and every method only has one left.

Let's start with simple_a1:

We can take the following block and extract it to a new method:

System.out.println("Request input for item " + i);
in_num = user_input.nextInt();
a1[i] = in_num;
System.out.println("Got it, next one ");

This does exactly one thing: prompt the user for a number and store it in the array. We won't store it in the array in our sub-method, but we'll extract the prompting out. Let's create a new method:

private static int getNumber(Scanner input, int index) {
    System.out.println("Request input for item " + index);
    int result = input.nextInt();
    System.out.print("Got it");
    return result;
}

Perfect, so this has one responsibility. It's not the prettiest, but it will do. Next we have to implement this in our other method:

for (int i = 0; i < 5; i++) {
    a1[i] = getNumber(user_input, i);

    if (i + 1 < 5) {
        System.out.println(", next one");
    }
}

So now our input prompting is separated from our processing, that's good but we can do better.

The block is now something along the lines of:

int[] a1 = new int[5];
Scanner input = new Scanner(System.in);

for (int i = 0; i < 5; i++) {
    a1[i] = getNumber(input, i);

    if (i + 1 < 5) {
        System.out.println(", next one");
    }
}

Well this whole block has one responsibility: build an array. So we can extract that out further:

private static int[] buildArray(int size) {
    int[] result = new int[5];
    Scanner input = new Scanner(System.in);

    for (int i = 0; i < 5; i++) {
        result[i] = getNumber(input, i);

        if (i + 1 < 5) {
            System.out.println(", next one");
        }
    }

    input.close();

    return result;
}

Good, so now simple_a1 looks like:

private static void simple_a1() {
    int[] a1 = buildArray(5);

    System.out.println("The numbers in a1 are: ");

    for(int i2: a1){
        System.out.print(i2 + " ");
    }

    System.out.println("\nEnd of testing 1, back to main menu...");
}

But we still have three remaining responsibilities:

  1. Build an array;
  2. Print the array;
  3. Inform the user we're going back to the main menu;

Well we can make a printArray method:

private static void printArray(int[] printArray) {
    for (int val : printArray) {
        System.out.print(val + " ");
    }
}

Then we finally finish simple_a1:

private static void simple_a1() {
    int[] a1 = buildArray(5);
    printArray(a1);
    System.out.print("\nEnd of testing 1");
}

So we're down to one responsibility: process an array. It's made up of steps, but the implementation for those steps is not part of simple_a1.

Next we'll fix simple_a2:

private static void simple_a2() {
    System.out.print("This function still under construction");
}

So we don't care where we return to, we only care that we're returning.

Finally, fixing main is easy:

public static void main(String[] args) {
    System.out.println("Welcome to basic array test, please choose your testing subject: ");
    Scanner input = new Scanner(System.in);

    mainMenu(input);

    System.out.println("Program terminated...");
    input.close();
}

private static void mainMenu(Scanner input) {
    int option = 0;

    do {
        System.out.println("Option 1: Simple one dimensional array.");
        System.out.println("Option 2: Simple two dimensional array. (under construction)");
        System.out.println("Option 0: Exit program");

        option = user_option.nextInt();
        System.out.println("current option is: " + option);

        switch(option) {
            case 1:
                //simple array input method 
                simple_a1();
                break;
            case 2:
                simple_a2();
                break;
            case 0:
                return;
            // In C# the `case else` is `default`, not sure what Java's is
            default:
                System.out.print("Invalid option: " + option);
                break;
        }

        System.out.println(", back to main menu");
    } while (option != 0);
}

We've got all our major components separated, and we can clearly see what each section is supposed to do.


Overall, good start, I hope to see more from you here. :)

\$\endgroup\$
5
\$\begingroup\$

try-with-resources

Since Java 7, you should use try-with-resources for safe and efficient handling of your underlying I/O resources, i.e. System.in. This means you do not have to re-create a Scanner wrapper around it, or to explicitly call its close() method in the end.

Naming/styling convention

Java's default naming convention is camelCase, instead of snake_case. It's more important to keep to one convention consistently though, so it's fine within this question. :)

You may also want to give your methods more descriptive names, as simple_a1()/simple_a2() does not convey much information.

I also find that there's too much vertical whitespace in your code, it will be more readable if you can remove all the empty lines.

\$\endgroup\$
  • 1
    \$\begingroup\$ I agree with the white space suggestion, although I think that in this case it is a result of breaking the SRP, as demonstrated by EBrown's answer. \$\endgroup\$ – carlossierra Feb 26 '17 at 14:42
2
\$\begingroup\$

Everything EBrown said with this ... minor exception ... the input close should precede the announcement that the program is done. I can not add comments or I would have simply made it a comment suggesting to EBrown the change.

public static void main(String[] args) {
    System.out.println("Welcome to basic array test, please choose your testing subject: ");

    Scanner input = new Scanner(System.in);

    mainMenu(input);

    input.close();

    System.out.println("Program terminated...");
}
\$\endgroup\$
  • \$\begingroup\$ Combine this with my answer and the one from h.j.k. and this programme becomes substantially better. :) +1 \$\endgroup\$ – Der Kommissar Feb 26 '17 at 20:44

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.