2
\$\begingroup\$

Just looking for some feedback regarding how to optimize my code. It works, but just want some feedback on how to improve the solution.

Requirements:

  1. Prompt for and read a number between 1 and 5. Repeat this step until the input is 1..5.

  2. Repeat the following multiple times according to the number read in step 1.

    • a. Read in a list of integers ending with a 0. The 0 marks the end of the input and is not considered part of the list
    • b. Print the largest and smallest integers in the list.
    • c. If only a zero appears in the list, print an error message


CODE:

import java.util.ArrayList;
import java.util.Collections;
import java.util.Scanner;

public class SIMPLE_NUMBER_PROGRAM {

    public static void main(String[] args) {

        int input = prompt();
        ArrayList<Integer> inputList;
        inputList = new ArrayList();
        Integer min;
        Integer max;

        if (input == 0 && inputList.isEmpty()) {
            System.out.println("error: empty list!");
        } else {
            while (input < 1 && input != 0 || input > 5 && input != 0) {
                input = prompt();
            }
            while (input >= 1 && input <= 5) {
                inputList.add(input);
                input = prompt();
            }
            min = Collections.min(inputList);
            max = Collections.max(inputList);
            System.out.println("Min: " + min + "\n" + "Max: " + max);
        }

    }

    private static int prompt() {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter a number: ");
        return sc.nextInt();
    }

}
\$\endgroup\$

migrated from stackoverflow.com Mar 12 '17 at 12:01

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ are you sure this works? you can easily input an infinite amount of numbers... is that by design? and what do you mean read input from user until the input is 1..5? @scullynbones \$\endgroup\$ – RAZ_Muh_Taz Mar 10 '17 at 23:00
  • \$\begingroup\$ @RAZ_Muh_Taz I'm a noob so please forgive my ignorance. These requirements were provided to me. I agree there is some ambiguity in how to interpret them. My understanding is the program will ask the user to enter numbers between 1-5. Each number entered between 1..5 is added to the list. Once 0 is entered the list ends. If the user enters a number that is not 1..5 or not 0 (for example, 8) it should ignore the number and ask them again to enter a number 1..5, until 0 is entered. So the user can enter an infinite amount of numbers until 0 is entered. \$\endgroup\$ – scullynbones Mar 10 '17 at 23:08
0
\$\begingroup\$

You really only need 1 while loop that loops on the condition that your input != 0. Then as you get your input you either add it to your list if the input is between 1-5, ignore all other values and when a 0 is entered the looping exits and you can determine max,min if possible. Try this

    ArrayList<Integer> inputList = new ArrayList();
    Integer min;
    Integer max;
    int input = -1;

    while(input != 0)
    {
        input = prompt();
        if(input > 0 && input < 6)
        {
            inputList.add(input);
        }
    }

    if(inputList.size() == 0)
        System.out.print("Error, No Valid Numbers Entered");
    else
    {
        min = Collections.min(inputList);
        max = Collections.max(inputList);
        System.out.println("Min: " + min + "\n" + "Max: " + max);
    }
\$\endgroup\$
1
\$\begingroup\$

If you make the Scanner object outside of the prompt method and use that object in the prompt method when you want to get an input then you won't be creating a new instance of Scanner every time you call the prompt method.

\$\endgroup\$
1
\$\begingroup\$

A couple of things I would add.

You don't actually need the collection here as it is trivial to do the check of whether the new number is the new max or min in the body of the loop.

SIMPLE_NUMBER_PROGRAM is not Java convention for a class name, this should be changed to SimpleNumberProgram.

The prompt method is creating a new instance of a scanner every call. The program could be restructured so the same scanner could be used.

Instead of just printing out your error case, it would be more inline with Java convention to throw an unchecked exception here, terminating the main thread and bringing your program to a halt. It also ensures your process terminates with the correct exit code. Look into exit codes and exceptions.

//Name uses proper java convention
public class SimpleNumberProgram {

    //Proper convention to have constants all upper case, not class names
    //Use the same instance for every iteration of our do loop (ie don't call new each time)
    private final static Scanner SCANNER = new Scanner(System.in);

    public static void main(String[] args) {
        //Integer.MIN_VALUE and INTEGER.MAX_VALUE are static constants (Note convention)
        //on the Integer class, that denote the highest possible and lowest possible integer values
        int max = Integer.MIN_VALUE;
        int min = Integer.MAX_VALUE;
        int input;
        do {
            System.out.println("Enter a number: ");
            input = SCANNER.nextInt();
            if (input >= 1 && input <= 5) {
                //Math max takes the max of the two inputs
                //remember we start with the Minimum possible integer value, so we will always take the input the first time.
                //Subsequent calls compare the old input to new input for the max
                max = Math.max(max, input);
                //Same for min but the inverse
                min = Math.min(min, input);
            }
        } while (input != 0);

        //Actually throw an exception here
        if (max == Integer.MIN_VALUE || min == Integer.MAX_VALUE) {
            throw new IllegalStateException("No Valid Numbers Entered");
        }
        System.out.println("Min: " + min);
        System.out.println("Max: " + max);
    }
}
\$\endgroup\$
0
\$\begingroup\$

Since this seems like the kind of tasks you get in the very beginning of introduction programming courses I don't really see the need for a code review at this stage.

But since you asked, it seems a bit messy on a lot of places. For instance, the isEmpty check is pointless, since at the time it is run, the list is always empty.

Depending on the goal of the assignment, it may or may not be necessary to use a list. If you just want to print the max and min, just store max and min and check for every new input if it changes. Something like:

if(input>max) max=input;

If you want to keep the list I would make it cleaner. Something like:

int input, min, max;
Scanner sc = new Scanner(System.in);
ArrayList<Integer> inputList;
inputList = new ArrayList<Integer>();
do {
        System.out.println("Enter a number: ");
        input = sc.nextInt();
        inputList.add(input);
} while(input>=1 && input <=5);
if(inputList.isEmpty()) {
    System.out.println("error: empty list!");
    System.exit();
}
min = Collections.min(inputList);
max = Collections.max(inputList);
System.out.println("Min: " + min + "\n" + "Max: " + max);

In this example, I don't really see the need for the method prompt. Either add more functionality into it, like the check for numbers being in range 1..5, or remove it completely.

\$\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.