Review of "Menu program"

Can anyone review my program and state where I could improve it, even in terms of comments, validation, etc?

```
package assessed_practical_2;

//Importing Resources (Random)
import java.util.Random;

//Importing Resources (Scanner)
import java.util.Scanner;

public class Assignment2 {

/**
* Scanner used for input within program
*/
public static Scanner scanner = new Scanner(System.in);

/**
* Main method that provides user with a menu in which each number
* represents a different method (e.g addtion) that they can carry out
*/
public static void main(String[] args) {

try {
// Declare variable for user's option and defaulting to 0
int menuOption = 0;
do {
// Setting menuOption equal to return value from showMenu();
menuOption = showMenu();

// Switching on the value given from user
switch (menuOption) {

case 1:
add();
break;
case 2:
subtract();
break;
case 3:
guessRandomNumber();
break;
case 4:
printLoop();
break;
case 5:
System.out.println("Quitting Program...");
break;
default:
System.out.println("Sorry, please enter valid Option");

}// End of switch statement

} while (menuOption != 5);

// Exiting message when user decides to quit Program
System.out.println("Thanks for using this Program...");

} catch (Exception ex) {
// Error Message
System.out.println("Sorry problem occured within Program");
// flushing scanner
scanner.next();
} finally {
// Ensuring that scanner will alwa

First thoughts:

You have this whole thing as all static. That's fine for a small simple program like this, but it might be better practice for you to make most of the methods into instance methods, and then in main(), instantiate it and call a driver method on that class. Static things can get messy in java when the logic gets more complicated. So, I would pull out most of the stuff that you have in your main method and put it into an instance method.

public static void main(String[] args) {
    Assignment2 assignment = new Assignment2();
    assignment.start();
}

Also, unless you're required to have your class named Assignment2, I would suggest changing it to something more descriptive (it should tell you what it does not what it is).

Might want to rename a few methods also. For example, showMenu() doesn't just show a menu, it also gets input from the user, so maybe rename that to something like getSelection() (or something).

Let's talk about your add() method.

do {
        num1 = random.nextInt(100);
        num1++;
        num2 = random.nextInt(100);
        num2++;

        do {
            validAnswer = true;

            System.out.println("Adding numbers...");
            System.out.printf("What is: %d + %d? Please enter answer below", num1,num2);
            result = num1 + num2;
            try {
                input = scanner.nextInt();
            } catch (Exception ex) {
                System.out.println("Sorry, Invalid entry for Addition...Please Retry!");
                scanner.next();
                validAnswer = false;
            }
        } while (!validAnswer);

Instead of assigning num1 and num2 on one line, and on the next, incrementing them, put that on one line.

num1 = random.nextInt(100) + 1;
num2 = random.nextInt(100) + 1;

Also, some minor issues with spacing. Change boolean correctAnswer=false; to boolean correctAnswer = false;

Also, it's bad practice to catch Exception. Catch and handle only the ones you expect to deal with -- If Java makes you surround something with a try catch, only catch the ones you need to catch and are prepared to handle. The way you're doing it, if something throws a null pointer exception, your program won't give you useful crash reporting. I think what you really want to catch is InputMismatchException. So, catch that or leave an explanation as to why you're swallowing all exceptions in a comment.

Your add() and subtract() methods are pretty much exactly the same. You should think about making them into one method that accepts an argument for which behaviour to exhibit.

You spend a lot of lines getting and validating input from the user. Think about writing a helper function to do that all in one place:

private int getInt(String message, int low, int high) {
    int num;
    boolean invalid = true;

    do {
        System.out.printf("Please enter an integer between %d and %d: ");

        try {
            num = scanner.nextInt();

            if (num >= low && num <= high) {
                invalid = false;
            }
        }
        catch (InputMismatchException x) {
            System.out.println("Invalid entry, please try again");
        }
    } while (invalid);
}

Something like that (if you need to use a do-while. Personally, I try to avoid them but it looks like you like them, and maybe it's part of your assignment).

Those are just a few things that I would suggest you think about. The main problems I see mostly relate to code duplication, so really think about ways to reduce that.

StackExchange Code Review Q#36436, answer score: 4