How to code menus and sub-menus

[Kendall Ponder]

What are your recommendations on the best way code a menu driven program? (I am not using Spring because I haven't learned it yet). The program will have a main program and depending on the users choice will have sub-menus until the user makes a final choice of what they want to do and then the class which performs that task will be called. Should I have a menu class with a sub-class for each particular menu? The way I have it now is I have an Input class with static methods which print out the menu. For example:

	public static final int mainMenuItemCount=4;

	public static void mainMenu(){
		System.out.println();
		System.out.println("Enter the number of your choice.");
		System.out.println();
		System.out.println("  1. Run a Runs per Game Senario.");
		System.out.println("  2. Run a Team competition Senario.");
		System.out.println("  3. Investigate Batting orders.");
		System.out.println("  4. Exit the program.");
	}
	

Each menu has its own ItemCount stored in the Input class. The Input class also has the following method which gets the users choice and returns the integer choice.

	public static int menuChoice(int menuItemCount){
		Boolean inputAccepted=false;
		Scanner scan = new Scanner(System.in);
		int choice=0;
		while(!inputAccepted){
			if(scan.hasNextInt()){
				choice=scan.nextInt();
				scan.nextLine();
				if(0<choice && choice<=menuItemCount){
					inputAccepted=true;
				}else{
					System.out.println("Please enter a number from 1 to "+menuItemCount+".");
				}
			}else{
				System.out.println("Please enter a number from 1 to "+menuItemCount+".");
			}
		}
		return choice;
	}

So the code which gets the main menu choice looks like this:

			Input.mainMenu();
			int choice=Input.menuChoice(Input.mainMenuItemCount);

			switch(choice){
				case 1: runsPerGame();
					break;
				case 2: teamCompetition();
					break;
				case 3: battingOrder();
					break;
				case 4: quit=true;
					break;
				default:

I think this would be a common programming task so I think it is important for me to learn the "best practice" way of handling it. I think the same principles would apply even if I wrote the program with a GUI interface. Should I have a mainMenu class and override toString instead of using a mainMenu method? Should my switch statement be hidden in an abstract factory? Any help would be appreciated!

[Marcus Biel]

Hi Kendall,

here's my advice:

I noticed you are using a lot of static methods. Try to avoid that, if not completly remove that. It seems to me, you have not yet the "hang of object oriented programming". Your code really looks a lot like C/C++ procedural code.
Try to think more in objects. Start with easy stuff, you don't necessarily need to throw in lots of patterns like an abstract factory.
You copy & pasted System.out.println() a lot of times. Instead, you could also just do one output, with a "\n" for a new line. (Have this static text as a private static final constant, that you can then printout).

Your menuChoice method is really horrible, sorry, it has so many different levels that it would be too time consuming for me to understand what it is trying to achieve. This is a big welcome for bugs to hide.
Simply the code, if not possible at all, use more sub methods, with 1-3 lines per private sub method, using a speaking name. Try to make the code speak to you, like a book.
Also try to avoid a switch. I am not sure if I understood you correctly, but it seems you have 4 choices, that will start different options.
Why not implement this in an object oriented someway like this (still to be improved, I am just thinking loud...)
(not necessarily correct, more like pseudo code):

public static void main(String args) {

Option option = new Option(args);
if(option.playGame()) {
playGame();
}
else if(option.teamCompetition()) {
teamCompetition();
}
else if(optionBattingOrder()) {
battingOrder();
}

Try to think more high level and before you code, use pen and paper to sketch a design of the different components comunicating with each other.
I see for example, you seem to read and write from and to the Console. Why not having a console object that you can call to write() and to read() from the console?
Alternatively, if things grow further, you could separate this into an input and output Channel, I mean having to separates Objects.
Then encapsulate your reading and writing logic into this / these objects. I know you will need some boilerplate code, just try to hide this nasty stuff as good as you can!
Also, I bet there is also some open source library / jar available somewhere that does exactly this - and then you can concentrate on your actual game,
and not on this boilerplate infrastructure code. The most important part should be your game logic, your code should make this one easily visible and understandable,
and should hide away everything else. That way, if at a later time you want to improve your game and have a proper ui, you could leave your game logic
untouched, and would just have to replace input / output objects that would then do a fancy UI for the user.

Hope that helps. I am working on a free Java Video course, which starts from scratch, but I give a lot of clean code advices like above - so maybe it's interesting to you.
I have the link in my signature.

Marcus

[Kendall Ponder]

Marcus,

Thanks for your reply. It will help me if I can respond to some of your points.

I noticed you are using a lot of static methods. Try to avoid that, if not completly remove that.

and

You copy & pasted System.out.println() a lot of times. Instead, you could also just do one output, with a "\n" for a new line. (Have this static text as a private static final constant, that you can then printout).

I agree about using \n. I had not looked up formatting when I wrote this last summer. I put all of the menus as static methods in the input class so it would be easy to add new menus and modify existing menus. I used static methods because I didn't see an advantage in having to instantiate the input class. Should I keep the input class and make the methods non-static or should each menu be its own class? If I make each menu its own class should I overwrite toString for each menu class and then just use System.out.print to print out the menu or should each class have a printMenu method? Or something else?

It seems to me, you have not yet the "hang of object oriented programming". Your code really looks a lot like C/C++ procedural code.

You are correct. I have been out of programming since about 1992 (I just retired as a teacher) and none of my training was in an OO language. I am posting this kind of question to help me get the hang of OO programming and I appreciate the time you (and others on other questions) have spent helping me.

Your menuChoice method is really horrible, sorry, it has so many different levels that it would be too time consuming for me to understand what it is trying to achieve. This is a big welcome for bugs to hide.
Simply the code, if not possible at all, use more sub methods, with 1-3 lines per private sub method, using a speaking name. Try to make the code speak to you, like a book.

Agreed. I have just finished the first part of Martin's "Clean Code" book and am trying to apply it to this code I wrote last summer. I think I know how break it up into smaller methods and make it easier to follow. I was really looking for advice on how to implement the menu logic and put the menuChoice method in to give context. I guess I should have cleaned it up first!

Also try to avoid a switch. I am not sure if I understood you correctly, but it seems you have 4 choices, that will start different options.
Why not implement this in an object oriented someway like this (still to be improved, I am just thinking loud...)
(not necessarily correct, more like pseudo code):

Why do you prefer the else-if to the switch? Since the branch depends on a single condition isn't the switch the natural choice? I could make a an Enum class Option and use the following code:

Option option = new Option(args);
switch(option)
case(PLAY_GAME)
playeGame();
break;
case(TEAM_COMPETION)
teamCompetition();
break;

and so on.

What is the advantage of the else-if code? The "Clean Code" book uses a general rule of hiding switch statements in an Abstract Factory and use them to make polymorphic objects. Does that apply here? My first choice would be to leave the logic here either in the form of the else-if or switch code but as you pointed out I am trying to make the transition to OO programming.

Alternatively, if things grow further, you could separate this into an input and output Channel, I mean having to separates Objects.
Then encapsulate your reading and writing logic into this / these objects. I know you will need some boilerplate code, just try to hide this nasty stuff as good as you can!

I was making an attempt to do that. The Input class was my attempt at an input object and it had all of the menus and the method that read the users choice and passed on the integer value.

Also, I bet there is also some open source library / jar available somewhere that does exactly this

I am sure you are correct, but the main point of writing this program was to get experience in as many aspects of Java code as possible.

Thanks again for your response. If you have time to respond some more I will appreciate it. Either way you have been helpful.

[Marcus Biel]

Lot's of questions. Hard to answer. There is not THE answer in design. I am sure I could come up with some object oriented design,
if I am truly spending time on this, but a) I am too lazy, and b) I doub't this would really help you.
I would ask you to step away from your computer, take a pen and paper, don't think in code like a machine, but think like a human would do about your problem,
then come up with names of real world objects that you will later map to class names. This will drive you into the right direction, and way from these big loops.
Do one thing at a time, and do it good. This also applies to code. One method only for mapping input Strings to Enums. This could be in a full fledged abstract factory,
if you know how to, but often enough just a factory method (which in this case MAY BE static should also be enough.
Also think of doing a look up on this, by extending your enum with the input String, then in a loop lookup for the correct enum. Alternatively, you can also set up a little
map, statically and use that for a lookup.

I later used an if else conditional, because I used to to call boolean methods on an object, I had created before hand.
I think this is more readable.

 if(option.playGame()) {
 	playGame();
 }

All these decisions of course depend on the size and complexity of a problem. You could even totally go without the if else statements by using a Strategy pattern, for example.
I am not saying you should use the Strategy pattern or any other pattern - this totally depends on the size and complexity of your problem.
Your first goal should be to have very very simple methods and just do one thing per method.

Your menuChoice method however seems to be crying for a state pattern maybe?
I guess I would put this Scanner / Reader logic into it's own object, that internally reads until the input is accepted,
and then just returns the correct input read. Oh, I just read it seems you did something like that, you talked about an "Input class". Hmm.
However, there I also see stuff like:

 0<choice && choice<=menuItemCount

Which is very hard to read for me.
I would think more of something like:

while(input.notAccepted()) {
	output.printInputCommand();
}

--And then, like before:

Option option = input.toOption();
if(option.playGame()) {
playGame();
}
else if(option.teamCompetition()) {
teamCompetition();
}
else if(optionBattingOrder()) {
battingOrder();
}

---
Regarding your parsing, it seems you have 2 invalid outputs,
one for the input beeing the wrong number, the second one for not beeing a number.
Why not just something like

private boolean isValidInput(String str) {
	return isNumberInMenuItemRange(toInt(str));
}

private boolean toInt(String str) {
	try {
	return Integer.parseInt(str);
	} catch(RutimeException e) {
		return -1;
	}
}

private boolean isNumberInMenuItemRange(int choice) {
	return choice > 0 && choice <= menuItemCount;
}

which you could further simplify with something like:

private boolean isNotNegative ..
private boolean isNotOutOfBounds...

( I guess I would not do this last step, I just want you to get in the right simplify mode...)

The 2 Clean Coder boooks are law to me!

p.s.: Don't try to me smart. In fact, try to be as stupid as possible ( I am quite good in this aspect! ;-) -
The times of beeing proud of 1000 lines methods no one can read are over. Try to write methods so small that even a non technical person can read it -
Talking to business guys, I usually copy and paste them excerpts of my code and ask them if that is correct from a business point of view.
If the code looks something like:

command.readOption().playGame(); this is possible.

For something like:

    while(!inputAccepted){
        if(scan.hasNextInt()){
            choice=scan.nextInt();
            scan.nextLine();
            if(0<choice && choice<=menuItemCount){
                inputAccepted=true;

It's not.

[Kendall Ponder]

Thanks for the feedback!

[Marcus Denker]

Hi Kendall,

please keep me updated with your progress on the matter, as I am highly interested how you would incorporate my feedback into your code

Thanks,

Marcus

[Kendall Ponder]

Marcus Denker wrote:Hi Kendall,

please keep me updated with your progress on the matter, as I am highly interested how you would incorporate my feedback into your code

Thanks,

Marcus

Here is the code after the first round of cleaning it up. I am looking forward to your comments (and to anyone else's comments who is still reading this thread). I left out the package and import statements. The programs in the switch statement are just stubs right now so I did not put them in. The code works but that doesn't mean it is good code!

public class Baseball {

	public static void main(String[] args)  {
		MainMenu mainMenu = new MainMenu();
		mainMenu.runMainProgram();
	}
}

public interface   Menu {

	String getMenuChoices();
	int getNumberOfMenuChoices();

}

public class MainMenu implements Menu {
	
	private String menuChoices;
	private int numberOfMenuChoices;
	
	enum MainMenuChoices  {SINGLE_TEAM_SENARIO, TWO_TEAM_SENARIO, MODIFY_TEAM, CREATE_TEAM, QUIT}

	
	public MainMenu() {
		menuChoices= String.format("%n%s%n%n%s%n%s%n%s%n%s%n%s%n",
				"Enter the number of your choice.",
				"  1. Run a Single Team Senario.",
				"  2. Run a Two Team Senario.",
				"  3. Modify an Existing Team.",
				"  4. Create a new Team.",
				"  5. Exit the program.");
		numberOfMenuChoices=5;

	}
		
	 public void runMainProgram() {
		 Menu mainMenu = new MainMenu();
		 MainMenuChoices mainMenuChoice;
			do{
				mainMenuChoice = getMainMenuUserChoice(mainMenu);
				runUsersChoice(mainMenuChoice);
			}while(userHasNotEnteredQuit(mainMenuChoice));
	}

	private MainMenuChoices getMainMenuUserChoice(Menu mainMenu) {
			Input userChoice = new Input();
			int mainMenuUserChoice = userChoice.menuChoice(mainMenu);  //Input class is on line 96
			return MainMenuChoices.values()[mainMenuUserChoice];
		}

	private void runUsersChoice(MainMenuChoices mainMenuChoice) {
			switch(mainMenuChoice){
				case SINGLE_TEAM_SENARIO: 
					SingleTeam singleTeam = new SingleTeam();
					singleTeam.runSingleTeamMenu();
					break;
				case TWO_TEAM_SENARIO: 
					TwoTeam twoTeam = new TwoTeam();
					twoTeam.runTwoTeamMenu();
					break;
				case MODIFY_TEAM: 
					TeamModifier teamModifier = new TeamModifier();
					teamModifier.runModifyTeamMenu();
					break;
				case CREATE_TEAM: 
					TeamCreator teamCreator = new TeamCreator();
					teamCreator.runTeamCreatorMenu();
					break;
				case QUIT:
					break;
				default:
			}	
		}			

	private boolean userHasNotEnteredQuit(MainMenuChoices mainMenuChoice) {
		return !(mainMenuChoice==MainMenuChoices.QUIT);
	}


	@Override
	public String getMenuChoices() {
		return menuChoices;            // Initialized on line 26
	}


	@Override
	public int getNumberOfMenuChoices() {
		// TODO Auto-generated method stub
		return numberOfMenuChoices;
	}
}

public class Input {

	private int INVALID_MENU_CHOICE = -1;
	private int menuChoice;
	
	public int menuChoice(Menu menu){
		do{
			System.out.print(menu.getMenuChoices());  //line 83
			menuChoice = getInteger();
		}while(menuChoiceIsInvalid(menu));
		return menuChoice-1; //Adjust for zero index of array
	}

	private int getInteger() {
		int userChoice;
		try{
			Scanner scan = new Scanner(System.in);
			userChoice = scan.nextInt();
		}catch(InputMismatchException e){
			userChoice= INVALID_MENU_CHOICE;
		}
		return userChoice;
	}

	private boolean menuChoiceIsInvalid(Menu menu) {
		boolean inValid =!(0<menuChoice && menuChoice<=menu.getNumberOfMenuChoices());
		if (inValid)
			System.out.println("You did not enter an integer from 1 to "+menu.getNumberOfMenuChoices()+".");
		return inValid;
	}


}

[Marcus Biel]

A few fast comments:

enum MainMenuChoices  {SINGLE_TEAM_SENARIO, TWO_TEAM_SENARIO, MODIFY_TEAM, CREATE_TEAM, QUIT}

        menuChoices= String.format("%n%s%n%n%s%n%s%n%s%n%s%n%s%n",
                "Enter the number of your choice.",
                "  1. Run a Single Team Senario.",
                "  2. Run a Two Team Senario.",
                "  3. Modify an Existing Team.",
                "  4. Create a new Team.",
                "  5. Exit the program.");

How about having the enum in its own file, and adding the Strings to the enum values?
I feel they would belong together.

numberOfMenuChoices=5;

that would better be private static final, hm?
---
What do you need The interface Menu for?
Don't fall into "interface-itis" - Use an interface, when you need and use it. (Some might disagree, but I like it pragmatic small and simple.
You can refactor your code anytime.

Don't always create a reference.
Consider (just consider the possibility)

return MainMenuChoices.values()[new Input().menuChoice(mainMenu)];

Same here:

new TwoTeam().runTwoTeamMenu();

Consider using longer variables. I don't understand "TwoTeam".

throw new InvalidArgumentException(invalid argument:"+ argument);

for default switch case.

shorten boolean expression:

mainMenuChoice != MainMenuChoices.QUIT

consider removing "get" infront of getters. This is old java bean style which will easily lead you to program an "anemic domain model" (Martin Fowler)

Never leave auto code or auto comments in your code, that looks very unprofessional:

// TODO Auto-generated method stub

       if (inValid)
            System.out.println("You did not enter an integer from 1 to "+menu.getNumberOfMenuChoices()+".");

ALWAYS use the curly brackets!

[Kendall Ponder]

Thanks for your response. As always I will have some questions.

How about having the enum in its own file, and adding the Strings to the enum values?
I feel they would belong together.

I put the enum in this file because it was the only class to use them. What would be the advantage of adding the Strings to the enum values?

numberOfMenuChoices=5;

that would better be private static final, hm?

Yes.

What do you need The interface Menu for?

I have menus everywhere in this program and wanted to try to take advantage of polymorphism. I have refactored the code to actually use the interface. I will put it in the next post.

Don't always create a reference.
Consider (just consider the possibility)

return MainMenuChoices.values()[new Input().menuChoice(mainMenu)];

Same here:

new TwoTeam().runTwoTeamMenu();

Is it harder to follow without a reference? With experience is it just as easy to follow? Is this different from the train wreck code in the Clean Code book where he said to break them up? It does make the code more concise and if it is normal practice I can do it.

Consider using longer variables. I don't understand "TwoTeam"

Will do. Is there a risk they get too long?

throw new InvalidArgumentException(invalid argument:"+ argument);

for default switch case.

If the code compiles that error can't happen there can it? Isn't that the point of using the enums?

shorten boolean expression:

mainMenuChoice != MainMenuChoices.QUIT

Will do.

consider removing "get" infront of getters. This is old java bean style which will easily lead you to program an "anemic domain model" (Martin Fowler)

Isn't the "anemic domain model" a class with just data and getters/setters? I don't see how using or not using get as part of the method name will change the structure of the class. Doesn't it make the code easier to read? getItem tells me the method is going to return the Item. What would be a better way to name it?

Never leave auto code or auto comments in your code, that looks very unprofessional:

oops! Missed that one!

ALWAYS use the curly brackets!

Will do!

Again, thanks for taking the time to respond. I will post my refactored code in the next post.

[Kendall Ponder]

Here is the refactored code. I refactored the code before your response so I have only implemented some of your suggestions.

public interface   Menu {

	void printMenuChoices();
	int getNumberOfMenuChoices();
	void presentMenuToUser(Menu menu);

}

public class MainMenu implements Menu {
	
	private String menuChoices;
	private static final int numberOfMenuChoices=5;
	
	enum MainMenuChoices  {SINGLE_TEAM_SENARIO, TWO_TEAM_SENARIO, MODIFY_TEAM, CREATE_TEAM, QUIT}
	
	public MainMenu() {
		menuChoices= String.format("%n%s%n%n%s%n%s%n%s%n%s%n%s%n",
				"Enter the number of your choice.",
				"  1. Run a Single Team Senario.",
				"  2. Run a Two Team Senario.",
				"  3. Modify an Existing Team.",
				"  4. Create a new Team.",
				"  5. Exit the program.");

	}
		
	public void presentMenuToUser(Menu mainMenu) {
		 MainMenuChoices mainMenuChoice;
			do{
				mainMenuChoice = getMainMenuUserChoice(mainMenu);
				Menu usersChoice = getNewMenu(mainMenuChoice);
				usersChoice.presentMenuToUser(usersChoice);
			}while(userHasNotEnteredQuit(mainMenuChoice));
	}
	 
	private MainMenuChoices getMainMenuUserChoice(Menu mainMenu) {
			int mainMenuUserChoice = new Input().menuChoice(mainMenu);
			return MainMenuChoices.values()[mainMenuUserChoice];
		}

	private Menu getNewMenu(MainMenuChoices mainMenuChoice) {
		 Menu newMenu;
		 switch(mainMenuChoice){
		 case SINGLE_TEAM_SENARIO: 
			 newMenu = new SingleTeamMenu();
			 break;
		 case TWO_TEAM_SENARIO: 
			 newMenu = new TwoTeamMenu();
			 break;
		 case MODIFY_TEAM: 
			 newMenu = new ModifyTeamMenu();
			 break;
		 case CREATE_TEAM: 
			 newMenu = new CreateTeamMenu();
			 break;
		 default:
			 newMenu = new QuitMenu();
		 }	
		 return newMenu;
	 }

	private boolean userHasNotEnteredQuit(MainMenuChoices mainMenuChoice) {
		return mainMenuChoice!=MainMenuChoices.QUIT;
	}

	@Override
	public void printMenuChoices() {
		System.out.print(menuChoices);
	}

	@Override
	public int getNumberOfMenuChoices() {
		return numberOfMenuChoices;
	}
}

public class Input {

	private int INVALID_MENU_CHOICE = -1;
	private int menuChoice;
	
	public int menuChoice(Menu menu){
		do{
			menu.printMenuChoices();
			menuChoice = getInteger();
		}while(menuChoiceIsInvalid(menu));
		return menuChoice-1; //Adjust for zero index of array
	}

	private boolean menuChoiceIsInvalid(Menu menu) {
		boolean inValid =!(0<menuChoice && menuChoice<=menu.getNumberOfMenuChoices());
		if (inValid){
			System.out.println("You did not enter an integer from 1 to "+menu.getNumberOfMenuChoices()+".");
		}
		return inValid;
	}

	private int getInteger() {
		int userChoice;
		try{
			Scanner scan = new Scanner(System.in);
			userChoice = scan.nextInt();
		}catch(InputMismatchException e){
			userChoice= INVALID_MENU_CHOICE;
		}
		return userChoice;
	}
}

[Kendall Ponder]

I have a similar thread at http://www.java-forums.org/new-java/94814-how-decide-when-use-methods-classes.html#post406491 and posted the code for Modifying a Team if anyone wants to check it out. Any suggestions on either code is appreciated.