Question on if I should use a static method

[Kendall Ponder]

I have an Input class which has the method

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

which I use to print out menus and return the users choice. I never modify an Input item and find myself doing

new Input().menuChoice(menu);

Should I just make the menuChoice static? If I make it static could I use a static import so I could write

menuChoice(menu)

or does that make it too hard for someone to find where menuChoice is coming from? Thanks!

[Campbell Ritchie]

You are asking the question the wrong way round. You should be asking, “Is there any reason to make this method static?”

Remember static members of classes should be the exception. Make everything not static until you have a good reason to have something static. Why do you have the menuChoiceIsInvalid method in the Input class? Surely that goes with the Menu class? And why is that menuChoice method not in the Menu class, too? Why is the −1 in that method rather than in the Menu method, too?

[Kendall Ponder]

Campbell Ritchie wrote:You are asking the question the wrong way round. You should be asking, “Is there any reason to make this method static?”

Remember static members of classes should be the exception. Make everything not static until you have a good reason to have something static. Why do you have the menuChoiceIsInvalid method in the Input class? Surely that goes with the Menu class? And why is that menuChoice method not in the Menu class, too? Why is the −1 in that method rather than in the Menu method, too?

My reason to make it static is I find Input.menuChoice(menu) easier to follow than new Input().menuChoice(menu) but that may just be because I haven't been programming in Java very long. I think I orginally set up an Input class because I was trying to implement the MVC structure and put the Input Class in my view package. If I remember right when I tried to use an abstract Menu class as a super class of all of my menus I found I had to make the variables holding the number of menu items and the string used to print out the choices public and I thought that violated the principle of encapsulation so I wrote the Input menuChoice method to take any object implementing the Menu interface and use the objects getters to get the menu specific data needed to get the users choice.

I didn't put the -1 in the getInteger because that method only gets the integer entered by the user whereas menuChoice returns an integer based on a zero index.

So asking the question the right way why would I ever want to make a method static? Thanks!

[Campbell Ritchie]

What is wrong with myMenu.getChoice()?
If you are passing numbers starting with 1, let the menu object decide on the item. If you are printing items 1 2 3 then the Menu class knows what those numbers mean. Let the Menu object decide how to interpret those numbers.
If it says

12 Steak and Kidney pie

then the Menu object knows S&KP is No12. Let the Menu object receive the 12 and let the Menu work out how to get the pie.

[Kendall Ponder]

What is wrong with myMenu.getChoice()?
If you are passing numbers starting with 1, let the menu object decide on the item. If you are printing items 1 2 3 then the Menu class knows what those numbers mean. Let the Menu object decide how to interpret those numbers.

It is true that the Menu knows what to do with those numbers (that is how I have it coded) but in order to get the number myMenu.getChoice() has to 1) give the user their choices and 2) insure the user choice is valid. Both of these are different for each menu but the code to do it is basically the same. I was going to do

class abstract Menu{
  public String menuString;
  public int menuCount;
  public int getChoice(){
  //Implementation code
  }
public void implementMenu();
}

where the constructor of each sub class of Menu would initialize menuString and menuCount. I could then do something like

Menu newMenu = getNewMenu();
newMenu.implementMenu();

and use polymorphism to eliminate duplicate code. The code for getChoice would be inherited and the getNewMenu() would use it. I did not do it this way because I thought the public fields in the abstract class violated encapsulation. Is that not a problem?

When I used a Menu interface instead I wanted a place where I could put the getChoice code once so I put it in Input which could run the same code on any object which implemented the Menu interface. I am probably not explaining this very well. I will put the code below if you want to see an example of how I implemented a menu. Thanks!

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() {
		 MainMenuChoices mainMenuChoice;
			do{
				mainMenuChoice = getMainMenuUserChoice();
				Menu usersChoice = getNewMenu(mainMenuChoice);
				usersChoice.presentMenuToUser();
			}while(userHasNotEnteredQuit(mainMenuChoice));
	}
	 
		private MainMenuChoices getMainMenuUserChoice( ) {
			int mainMenuUserChoice = new Input().menuChoice(this);
			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;
	}
}

[Campbell Ritchie]

Kendall Ponder wrote: . . . 1) give the user their choices and 2) insure the user choice is valid. Both of these are different for each menu but the code to do it is basically the same. . . .

That proves it. The validation of choices and the getting of the item belong in the Menu class. Look up Single Responsibility Principle. An object should take care of itself and should not take care ofthe internal workings of objects from different classes.

I like the idea of an enum. Read more about enums in the Java® Tutorials. You can give each of those objects in the enum a method to get a new sort of Menu object. So you no longer need the switch‑case statement. You can get an array of objects from an enum with one of its methods. You can use a number as an array index with or without −1 to get that particular object from the array. You can print all the elements of the array with their indices with or without +1. You can override the toString method in each enum constant to return a nice name for it, and you can provide that nice name in its constructor. You can validate that the number entered is in the correct range simply by knowing you have to use it an an array index (allowing for any +1 or −1).

[Kendall Ponder]

Thanks for the feedback, this is much better! I did not override toString so if that would be better than how I did it let me know. Should I extract the getInterger() method and the fields it uses into an abstract Menu class so all of the menus can inherit it? It is the only part of this code which is not specific to a given menu. Thanks again for your time!

public interface   ExecutesMenu {
	void executeMenuChoice();
}

public class MainMenu implements ExecutesMenu {

	enum MainMenuChoices  { SINGLE_TEAM_SENARIO("  1. Run a Single Team Senario.", new SingleTeamMenu()),
		TWO_TEAM_SENARIO("  2. Run a Two Team Senario.", new TwoTeamMenu()), 
		MODIFY_TEAM("  3. Modify an Existing Team.", new ModifyTeamMenu()), 
		CREATE_TEAM("  4. Create a new Team.", new CreateTeamMenu()), 
		QUIT("  5. Exit the program.", new QuitMenu());

	private String promptMessage;
	private ExecutesMenu nextMenu;

		private MainMenuChoices(String promptMessage, ExecutesMenu nextMenu) {
			this.promptMessage=promptMessage;
			this.nextMenu = nextMenu;
		}

		public void printMenu(){
			System.out.format("%n%s%n%n", "Enter the number of your choice.");
			for(MainMenuChoices menuChoice: MainMenuChoices.values()){
				System.out.format("%s%n", menuChoice.promptMessage);
			}
		}
	}

	private static final int INVALID_MENU_CHOICE = -1;
	private int menuChoice;
	private int numberOfMenuChoices;
	
	public MainMenu() {
		this.numberOfMenuChoices=MainMenuChoices.values().length;
	}
	
	@Override
	public void executeMenuChoice() {
		ExecutesMenu newMenu = getMenuChoice();
		newMenu.executeMenuChoice();
	}

	public ExecutesMenu getMenuChoice(){
		MainMenuChoices sampleMainMenuChoice = MainMenuChoices.CREATE_TEAM;
		do{
			sampleMainMenuChoice.printMenu();
			menuChoice = getInteger();
		}while(menuChoiceIsInvalid());
		return MainMenuChoices.values()[menuChoice-1].nextMenu; //Subtract 1 to convert user input to zero based index
	}

	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() {
		boolean inValid = !(0<menuChoice && menuChoice<=numberOfMenuChoices);
		if (inValid){
			System.out.format("%s%s%s%n","You did not enter an integer from 1 to ",numberOfMenuChoices,".");
		}
		return inValid;
	}
}

[Campbell Ritchie]

Kendall Ponder wrote: . . . this is much better! . . .

Isn't it!

As a default behaviour you should override toString in every class you write. It is not a case of justifying overriding toString so much as a case of justifying not overriding toString.
Line 21: That method seems to behave the same whether it is called from a particular menu enum item or not. In which case it should maybe be static. You know about nested enums being implicitly static?
Line 57: Why are you returning −1? It is possible to avoid InputMismatchException completely in that sort of input, but I haven't got the time to explain it now. Search my posts for Rob Spoor because he taught me how to do it.
Line 49: There is separation of responsibility. The isInvalid method should tell you whether it is invalid, but there is something wrong about that method printing the error message too. That will require some thought which I am too tired to do now.
Line 66: If you print an error message use System.err not System.out.

[Kendall Ponder]

As a default behaviour you should override toString in every class you write. It is not a case of justifying overriding toString so much as a case of justifying not overriding toString.

So I should override toString for MainMenu class, not for the enum, correct?

Line 21: That method seems to behave the same whether it is called from a particular menu enum item or not. In which case it should maybe be static. You know about nested enums being implicitly static?

My line numbers don't match up with the ones you used for some reason so I'm not sure which method you mean. I think you mean the printMenu() method which will always do the same thing for any instance of MainMenu. We are now back to your original point that I should ask the question "Is there a reason to make the method static?". I'm not sure why I would want to use printMenu() without an instance of MainMenu. Also, if I don't make it static should I make it private and use it in my toString overide? I did not know nested enums were implicitly static. Does that mean the MainMenuChoices enum is available as soon as the MainMenu class is loaded? I think it would have to be if I made printMenu() a static method.

Line 57: Why are you returning −1? It is possible to avoid InputMismatchException completely in that sort of input, but I haven't got the time to explain it now. Search my posts for Rob Spoor because he taught me how to do it.
Line 49: There is separation of responsibility. The isInvalid method should tell you whether it is invalid, but there is something wrong about that method printing the error message too. That will require some thought which I am too tired to do now.

isInvalid tells me if the input is invalid but there are two ways that could happen, 1) the input is not an integer and 2) the integer input is not in the correct range. I thought isInvalid had one responsibility (tell me if the input is invalid) but had to check two things to accomplish that responsibility. By returning an integer outside the range (-1 will always be outside the range) I only have to have one loop to check if the input is valid and tell the user to reenter if necessary. Otherwise I need a loop inside of getInteger() to make sure it is an integer and I would still need another loop somewhere to make sure the integer is in range. I will look through your posts for Rob Spoor.

If you print an error message use System.err not System.out.

I need the user to see why I am asking them to re-enter their choice. If I send the message to System.err will they see it on the console?

As always I appreciate the time you are giving me. I hope you have a great Father's Day weekend!

[Campbell Ritchie]

Kendall Ponder wrote: . . .
So I should override toString for MainMenu class, not for the enum, correct?

No. If you ever need to print mainMenu, then you should override toString in both.

. . . I think you mean the printMenu() method . . .

Yes, I mean printMenu. If it is in the main menu class, that is better, but it might still be static. Are you ever going to have main menus which have different options? If so, that method can't be static.

. . . isInvalid tells me if the input is invalid . . .

In which case isInvalid should return a boolean. A method should do one thing. Returning a boolean is one thing, printing an error message is another. When you have found out how to avoid the input mismatch exception you can probably work out how to get the input always in the required range, so you can get the input in such a way that it will always be valid.

. . . If I send the message to System.err will they see it on the console?

Yes. Unless you have redirected System.err.

As always I appreciate the time you are giving me. I hope you have a great Father's Day weekend!

Thank you

[Kendall Ponder]

It is possible to avoid InputMismatchException completely in that sort of input, but I haven't got the time to explain it now. Search my posts for Rob Spoor because he taught me how to do it.

I found a post where you explained it.

Campbell Ritchie wrote:Welcome to JavaRanch

Rob Spoor points out it is completely possible to avoid the InputMismatchException like this, or something rather similar:

private Scanner input;
 . . .
int i;
 . . .
while (!input.hasNextInt())
{
   input.next();
   System.err.println("Incorrect entry: not a number");
}
i = input.nextInt();

The reason I did not do it that way was it would require two loops (with their attendant error message and print menu statement). The one using .hasNextInt() and one which checks for the correct range.

[Campbell Ritchie]

And what is wrong with that? You now have two methods which do one thing. One gets a number and the other checks it is in the correct range. Remember you only repeat the loops if the wrong input is given. You can also out such loops in methods in a utility class and use it again and again.

And I thought (over a drink) that if there is any doubt about whether method X should be static, rather some than somebody having found a good reason why it should be static:-

Make it not static.

[Kendall Ponder]

And what is wrong with that? You now have two methods which do one thing. One gets a number and the other checks it is in the correct range. Remember you only repeat the loops if the wrong input is given. You can also out such loops in methods in a utility class and use it again and again.

I find the logic and flow simpler to follow where getInteger() returns the integer the user entered or -1 if no integer is entered and then getMenuChoice checks the integer range and uses it if it is correct or prints out an error message and the menu and asks for the user input again. Then the error message and menu is only output in one place. I can get the error message out of the isInValid method either way. If you think the flow is better with two loops I will use your experience. Plus, when you write the code it is hard to know what is easier for someone else to follow the first time they read it.

So it would be ok to make getInteger() a static method in a utility class? If I want to push all of the checking logic into it I will have to pass the numberOfMenuChoices field to it which I assume will be OK. That was kind of my original idea with my Input class but I didn't implement it very well. Thanks again!

[Campbell Ritchie]

Kendall Ponder wrote: . . .
I find the logic and flow simpler to follow where getInteger() returns the integer the user entered or -1 if no integer is entered . . . .

But -1 is an integer. That is the problem with returning sentinel values.

If you are supposed to enter a number in the range 0…6 and you don't, what are you going to do? What if the input is 999, -1234567890, “Campbell Ritchie”, or whatever? Are you going to throw an Exception? Are you going to return a sentinel value? Are you going to ask for a repeat? Which of those actions is most likely to obtain an int in the result? When you searched my posts for Rob Spoor, you found exactly what I hoped you would find. Look at that loop. If you run it, will i always be an int? I would write a utility class with that loop inside a static method (no that isn't true: I already have done it). Yes. Good idea. You can put another method in the same utility class which verifies that the input is in the requisite range between min and max. Absolutely guaranteed to work nicely.

Yesterday I posted some peculiar grammar so I have corrected it in the underlined text in my previous post.

[Campbell Ritchie]

A few minutes ago, I wrote: . . . Absolutely guaranteed to work nicely. . . .

Except possibly if you manage to pass max less than min

I shall let you work out how to solve that problem.

[Kendall Ponder]

Campbell Ritchie wrote:

Kendall Ponder wrote: . . .
I find the logic and flow simpler to follow where getInteger() returns the integer the user entered or -1 if no integer is entered . . . .

But -1 is an integer. That is the problem with returning sentinel values.

If you are supposed to enter a number in the range 0…6 and you don't, what are you going to do? What if the input is 999, -1234567890, “Campbell Ritchie”, or whatever? Are you going to throw an Exception? Are you going to return a sentinel value? Are you going to ask for a repeat? Which of those actions is most likely to obtain an int in the result? When you searched my posts for Rob Spoor, you found exactly what I hoped you would find. Look at that loop. If you run it, will i always be an int? I would write a utility class with that loop inside a static method (no that isn't true: I already have done it). Yes. Good idea. You can put another method in the same utility class which verifies that the input is in the requisite range between min and max. Absolutely guaranteed to work nicely.

Yesterday I posted some peculiar grammar so I have corrected it in the underlined text in my previous post.

Is your issue with the way I am doing it that getInteger() only works with this code? I would not be able to use it in another program which could accept a -1. If I do it your way I can put it in a Utility class and then I can use it for the rest of my life with any code where I need to get an integer from the console. Thanks!

[Campbell Ritchie]

Kendall Ponder wrote: . . . put it in a Utility class and then I can use it for the rest of my life . . . Thanks!

yesyesyesyesyes … and in a few weeks you can enhance it and the enhancements will be available for you to use for ever

And … “You're welcome”

[Kendall Ponder]

I see says the blind man!! It makes sense to me now. Off to work on getting the code working. I have another related question on this code but I think it will work best if I incorporate what you have shown me so far before I ask it. Thanks for staying with me, this has been a long thread.

[Campbell Ritchie]

It's always worth it when you hear the penny drop.

[Kendall Ponder]

Here is what the MainMenu class looks like now. System.err and System.out did not play well together so I used System.out for all of my messages. My question is I have a ModifyTeam menu where I have to pass a Team object to whichever class the user chooses. I don't know what the team is when ModifyTeam is called so I can't put the Team in the enum construction call. I have found a way to do it but I wondered if there was a "best practice" way to do it? Thanks!

public class MainMenu implements ExecutesMenu {

	enum MainMenuChoices  { SINGLE_TEAM_SENARIO("  1. Run a Single Team Senario.", new SingleTeamMenu()),
		TWO_TEAM_SENARIO("  2. Run a Two Team Senario.", new TwoTeamMenu()), 
		MODIFY_TEAM("  3. Modify an Existing Team.", new ModifyTeamMenu()), 
		CREATE_TEAM("  4. Create a new Team.", new CreateTeamMenu()), 
		QUIT("  5. Exit the program.", new QuitMenu());

	private String promptMessage;
	private ExecutesMenu nextMenu;

		private MainMenuChoices(String promptMessage, ExecutesMenu nextMenu) {
			this.promptMessage=promptMessage;
			this.nextMenu = nextMenu;
		}

		public void printMenu(){
			System.out.format("%n%s%n%n", "Enter the number of your choice.");
			for(MainMenuChoices menuChoice: MainMenuChoices.values()){
				System.out.format("%s%n", menuChoice.promptMessage);
			}
		}
	}

	private int menuChoice;
	private int numberOfMenuChoices;
	
	public MainMenu() {
		this.numberOfMenuChoices=MainMenuChoices.values().length;
	}
	
	@Override
	public void executeMenuChoice() {
		ExecutesMenu newMenu;
		do{
			newMenu = getMenuChoice();
			newMenu.executeMenuChoice();			
		}while(!newMenu.equals("QUIT"));
	}

	public ExecutesMenu getMenuChoice(){
		MainMenuChoices sampleMainMenuChoice = MainMenuChoices.CREATE_TEAM;
		sampleMainMenuChoice.printMenu();
		menuChoice = Input.getIntegerFromMinToMax(1, numberOfMenuChoices);
		return MainMenuChoices.values()[menuChoice-1].nextMenu; //Subtract 1 to convert user input to zero based index
	}

}

public class Input {

	public static int getInteger() {
			Scanner scan = new Scanner(System.in);
			while(!scan.hasNextInt()){
				scan.next();
				System.out.println("The value you entered is not an integer.  Please try again.");
			}
			 return scan.nextInt();
		}
	
	public static int getIntegerFromMinToMax(int minimumNumberAccepted, int maximumNumberAccepted){
		int inputFromUser = getInteger();
		while(inputFromUser<minimumNumberAccepted || maximumNumberAccepted<inputFromUser){
			System.out.println("The value you entered is not from "+
								minimumNumberAccepted+" to "+
								maximumNumberAccepted+".  Please try again.");
			inputFromUser = getInteger();
		}
		return inputFromUser;
		
	}

}

[Campbell Ritchie]

A lot better isn't it.

Maybe the method to print the options should be static because you may have to call it before any menu objects are created.
You can dispense with the numbers in the enum Strings because each enum element has a number associated which you can obtain with its ordinal() method. The first element is no 0, but you already know about +1 and −1.
The way you have arranged that enum, you will always get the same specialised menu object if you repeat a selection. I don't think that is a problem in this particular situation.
In the input class make the Scanner object a static final field rather than a local variable. You only ever need one instance. But beware: that arrangement is not thread‑safe. Maybe call both methods getInt rather than getInteger, (or findInt or inputInt or similar) because they are returning an int not an Integer. You can distinguish them because they are overloaded and have different numbers of parameters, so there is no risk of confusion. The only thing you are risking confusing is max and min the wrong way round.
Consider enhancing those methods with a String for the request message.

[Kendall Ponder]

A lot better isn't it.

Yes! I actually had several menus fully functional last summer before I knew anything about Clean Code or OO coding. I was going to clean up the code I wrote for modifying the batting order and use it in this version but after looking at it I found it was easier to just write it over from scratch. Quite a change!

Maybe the method to print the options should be static because you may have to call it before any menu objects are created.

Sounds good to me, is there a disadvantage to making it static?

You can dispense with the numbers in the enum Strings because each enum element has a number associated which you can obtain with its ordinal() method. The first element is no 0, but you already know about +1 and −1

That would make it simpler to modify the menu order, but in Effective Java, Bloch says to never derive a value associated with an enum with its ordinal. Does that not apply here since I know the ordinal will always give me the correct value and there are no duplicate values?

The way you have arranged that enum, you will always get the same specialised menu object if you repeat a selection. I don't think that is a problem in this particular situation.

Not sure I understand this point. If you are saying every time the user chooses 3 they will get the ModifyTeam class that is correct and is what I want unless there is a problem doing it that way.

In the input class make the Scanner object a static final field rather than a local variable. You only ever need one instance. But beware: that arrangement is not thread‑safe

Will do.

Maybe call both methods getInt rather than getInteger, (or findInt or inputInt or similar) because they are returning an int not an Integer. You can distinguish them because they are overloaded and have different numbers of parameters, so there is no risk of confusion. The only thing you are risking confusing is max and min the wrong way round.

Changing Integer to Int is clarifying but isn't changing getIntFromMinToMax to an overloaded getInt less clear? Plus getIntFromMinToMax helps the user know the Min is the first argument which helps with one of the reasons the Clean Code book said to avoid two argument methods where possible.

Consider enhancing those methods with a String for the request message.

I didn't put a request message in so they could be more general purpose, but I think you mean to pass a request message to the methods. In my case I want to print the menu as the request message but other times I will want a different message.

As always I appreciate your time. Because of people like you on this board when I looked at the table of contents on the Effective Java book I had an idea of what issues each section was referring to. That definitely was not true last summer.

[Campbell Ritchie]

Kendall Ponder wrote: . . . before I knew anything about Clean Code or OO coding. . . .

A bit more practice and you can be as objectionable as me.

Sounds good to me, is there a disadvantage to making it static?

Can't think of one at the moment.

. . . in Effective Java, Bloch says to never derive a value associated with an enum with its ordinal. Does that not apply here . . .?

Don't know. Please supply the page number and I'll have a look in my copy.

Not sure I understand this point. If you are saying every time the user chooses 3 they will get the ModifyTeam class that is correct . . .

No, if you choose 3 twice in the same execution you get the same ModifyTeam object. I still think that will behave correctly.

. . . but isn't changing getIntFromMinToMax to an overloaded getInt less clear? . . .

Maybe yes. Isn't that what documentation comments are there for? Let's see what other people say, and see what the range of opinions is

. . . I think you mean to pass a request message to the methods. In my case I want to print the menu as the request message but other times I will want a different message.

As always I appreciate your time. . . .

You're welcome.

And, yes, I meant that you can pass a message like "Please enter size: " to the getXXX methods of your input class. In the current situation you can pass the menu output as a request message. That is how I wrote my kyeboard inputs class; as you can see there are several different ways to get the same solution.

[Campbell Ritchie]

Is that Effective Java™ page 158?
There Bloch shows an enum like this

public enum Ensemble
{
    SOLO,   DUET,   TRIO,  QUARTET, QUINTET,
    SEXTET, SEPTET, OCTET, NONET,   DECTET ;
    
    public int numberOfMusicians()
    {
        return ordinal() + 1;
    }
}

He then explains that you cannot add a double quartet (8) because it is impossible to get a second value with 8 (octet). Nor can you add a triple quartet (12) unless you have a dummy value to consume the position for 11 players.

I think the current situation is slightly different. You are actually trying to obtain the enum constant using its ordinal to get an array index. As an alternative, you can consider the enum's valueOf method which all enums support. That allows you to pass the name of the constant as a String. But if you pass the wrong spelling you are liable to get an illegal argument exception.

[Campbell Ritchie]

You need to read four pages more. On page 162 Bloch says that what we have works but is error‑prone and I think he will provide a solution there.

[Kendall Ponder]

Campbell Ritchie wrote:Is that Effective Java™ page 158?
There Bloch shows an enum like this

public enum Ensemble
{
    SOLO,   DUET,   TRIO,  QUARTET, QUINTET,
    SEXTET, SEPTET, OCTET, NONET,   DECTET ;
    
    public int numberOfMusicians()
    {
        return ordinal() + 1;
    }
}

He then explains that you cannot add a double quartet (8) because it is impossible to get a second value with 8 (octet). Nor can you add a triple quartet (12) unless you have a dummy value to consume the position for 11 players.

I think the current situation is slightly different. You are actually trying to obtain the enum constant using its ordinal to get an array index. As an alternative, you can consider the enum's valueOf method which all enums support. That allows you to pass the name of the constant as a String. But if you pass the wrong spelling you are liable to get an illegal argument exception.

That is the part I was referring to.

You need to read four pages more. On page 162 Bloch says that what we have works but is error‑prone and I think he will provide a solution there.

I am using a Safari Online version so I don't have a page number but I think you are refering to item 33 where he says to use EnumMap instead of ordinal to index an array. But we aren't going to use ordinal to index an array, just to print out the line number of the menu and the order of the enums will always match the order of the line numbers. It will actually make it simpler to modify the menu. I recently added a capability to a menu and had to change all of the line numbers by hand and this would eliminate that. It can't cause an error. The worst that happens is the menu doesn't print out right and that would be easy to debug. So I think your ordinal idea is good and is an exception to the rule.

Reading the Effective Java Enum section gave me an idea on why we avoid using the switch like I originally did. It isn't that the original code is hard to follow (it is pretty obvious what a switch does) but it is harder to maintain when you start adding capabilities. Is that in the ball park?

[Kendall Ponder]

Not sure I understand this point. If you are saying every time the user chooses 3 they will get the ModifyTeam class that is correct . . .

No, if you choose 3 twice in the same execution you get the same ModifyTeam object. I still think that will behave correctly.

Got it because the new object is loaded once when the enum is constructed, so the user keeps choosing the same object. I will have to keep that in mind down the road.

[Kendall Ponder]

Campbell Ritchie wrote:

. . . but isn't changing getIntFromMinToMax to an overloaded getInt less clear? . . .

Maybe yes. Isn't that what documentation comments are there for? Let's see what other people say, and see what the range of opinions is

The Clean Code book says comments always represent a failure to express yourself in the code (pg 54) and should be used as a last resort. If at all possible the name should replace the comment. This may be a beauty is in the eye of the beholder case. I started coding when you only had 8 characters for a variable name so it has taken some getting used to the long variable names. I may go overboard with it.

[Campbell Ritchie]

All methods should have documentation comments. Well, all non‑private methods anyway. That is not so much to explain the logic as to state a general contract for that method. Like

if you pass me a message and two numbers I shall ensure you obtain an int from the keyboard between those two numbers.

But if you give me max smaller than min I shall throw a hissy fit Exception.

Remember that users of the method will often not be able to see the code.

Yes, that is the part of EJ I was thinking of. (No page numbers ?) Bloch does say that using ordinal to select from the array does work, but use of the enum map would be better. I didn't have time to read the rest of the section. Shall look later.
Yes, if you modify the enum in the future, as long as you don't hard‑code the max and min for the method to get the index from the keyboard, you will still get indices which correspond to the menu elements. If the enum is used elsewhere, however, there is a risk of breaking code in that “elsewhere”. Yes, I think you have got the idea there

[Kendall Ponder]

Here is the code with the modifications. MainMenu now loads all of the teams currently available to use from a file and when the user is finished saves any changes to the list back to the file. I got rid of the printMenu() method in the enum and just overrode the toString() method in the enum. I then used the toString() enum method to build the menu and passed the menu to the getIntFromMinToMax() method. I didn't use .ordinal but I did take the line numbers out of the enum strings and put them in manually in the buildMenuDisplay() method. I think that will make the code easier to maintain. I also overrode the toString() method in MainMenu to print the menu for use in the future if needed. There is also a getYesOrNoFromTheUser() method in the Input class I use later in the program. I think the changes have made the code cleaner.

package controller;

public class Baseball {

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

public class MainMenu implements ExecutesMenu {

	enum MainMenuChoices  { 
		SINGLE_TEAM_SENARIO(".  Run a Single Team Senario.", new SingleTeamMenu()),
		TWO_TEAM_SENARIO(".  Run a Two Team Senario.", new TwoTeamMenu()), 
		MODIFY_TEAM(".  Modify an Existing Team.", new ModifyTeamMenu()), 
		CREATE_TEAM(".  Create a new Team.", new CreateTeamMenu()), 
		QUIT(".  Exit the program.", new QuitMenu());

		private String promptMessage;
		private ExecutesMenu nextMenu;

		private MainMenuChoices(String promptMessage, ExecutesMenu nextMenu) {
			this.promptMessage=promptMessage;
			this.nextMenu = nextMenu;
		}

		@Override
		public  String toString(){		
			return this.promptMessage;
		}
	}

	public static ArrayList<Team> listOfAvailableTeams;
	private int menuChoice;
	private int numberOfMenuChoices;
	private String menuDisplay;
	
	public MainMenu() {
		this.numberOfMenuChoices=MainMenuChoices.values().length;
		this.menuDisplay = buildMenuDisplay();
	}
	
	private String buildMenuDisplay() {
		StringBuilder menuDisplay = new StringBuilder("Enter the number of your choice.");
		menuDisplay.append(System.lineSeparator()).append(System.lineSeparator());
		int lineNumber = 1;
		for(MainMenuChoices menuChoice: MainMenuChoices.values()){
			menuDisplay.append(lineNumber++).append(menuChoice.toString()).append(System.lineSeparator());
		}
		return menuDisplay.toString();
		}

	@Override
	public  String toString(){		
		return this.menuDisplay;
	}
	
	@Override
	public void executeMenuChoice() {
		loadAllAvailableTeams();
		ExecutesMenu newMenu;
		do{
			newMenu = getMenuChoice();
			newMenu.executeMenuChoice();			
		}while(!newMenu.equals("QUIT"));
		storeAllAvailableTeams();
	}

	private void loadAllAvailableTeams() {
		listOfAvailableTeams = new TeamStorage().retriveExistingTeams();
	}

	private ExecutesMenu getMenuChoice(){
		menuChoice = Input.getIntegerFromMinToMax(1, numberOfMenuChoices,this.menuDisplay);
		return MainMenuChoices.values()[menuChoice-1].nextMenu; //Subtract 1 to convert user input to zero based index
	}

	private void storeAllAvailableTeams() {
		new TeamStorage().storeAllTeamsInFile(listOfAvailableTeams);
	}

}

public class Input {
	static final Scanner scan = new Scanner(System.in);

	public static int getInteger(String promptMessage) {
		System.out.print(promptMessage);
			while(!scan.hasNextInt()){
				scan.next();
				System.out.println("The value you entered is not an integer.  Please try again.");
				System.out.print(promptMessage);
			}
			 return scan.nextInt();
		}
	
	public static int getIntegerFromMinToMax(int minimumNumberAccepted, int maximumNumberAccepted, String promptMessage){
		int inputFromUser = getInteger(promptMessage);
		while(inputFromUser<minimumNumberAccepted || maximumNumberAccepted<inputFromUser){
			System.out.println("The value you entered is not from "+
								minimumNumberAccepted+" to "+
								maximumNumberAccepted+".  Please try again.");
			inputFromUser = getInteger(promptMessage);
		}
		return inputFromUser;
		
	}
	
	public static String getYesOrNoFromTheUser(String promptMessage){
		System.out.format("%n%s%n", promptMessage);
		String userInput = getLowerCaseUserInput();
		if(userEnteredInvalidChoice(userInput)){
			System.out.format("%n%s%n", "You must enter Y, y, N or n.");
			userInput = getYesOrNoFromTheUser(promptMessage);
		}
		return userInput;
	}
	
	private static String getLowerCaseUserInput() {
		String userInput = scan.next();
		return userInput.toLowerCase();
	}
	
	private static boolean userEnteredInvalidChoice(String userInput) {
		return !(userInput.equals("y") || userInput.equals("n"));
	}
}

[Campbell Ritchie]

Not bad Only a few things:-

I had hoped to run your code, but there seem to be classes which I haven't got.
Your indentation is inconsistent in a few places. Be sure to be consistent about how much whitespace you have before each {. You can cheat by copying and pasting the entire class onto an Eclipse window and using ctrl‑A→ctrl‑I.
There is an inconsistency between the yes or no method and the invalid choice method.
Many of your lines are long. I think the longest is about 119 keystrokes long, which you can just about get away with, but it would look better if the lines were shorter. If you change this:-
        return MainMenuChoices.values()[menuChoice-1].nextMenu; //Subtract 1 to convert user input to zero based
to this:-
        //Subtract 1 to convert user input to zero based
        return MainMenuChoices.values()[menuChoice-1].nextMenu;
… you can see how much shorter the lines are
You can break lines after commas, etc.

Why is the List of Teams static? And even more, why does it not have private access?
Move the Input class into a different package; you might call it resources. Then it will be there for all other classes to use via an import, or even a static import.
The scan field should have private access (but if you move the class to a different package scan will become inaccessible).
Give the Input class a private no‑arguments constructor, for reasons described in the Java® language Specification (=JLS). That is one of the easier parts of the JLS to understand.

And isn't it better now than at first

[Kendall Ponder]

Why is the List of Teams static? And even more, why does it not have private access?

I made it static because I only wanted one master list every time the program was run and made it public because I wanted any changes made by any classes to immediately show up in this list.

[Campbell Ritchie]

So every time you run the application you always want the same list of teams? You never have different leagues with different teams in?

[Kendall Ponder]

Campbell Ritchie wrote:So every time you run the application you always want the same list of teams? You never have different leagues with different teams in?

Yes. This program is not a fantasy league program where you draft players and make up leagues and teams. The purpose of the program is to find out the impact different strategies or different kinds of players have, eg. if all of the other players are the same how does a 0.200 batting average player with 30 home runs and never steals impact the number of runs scored per game versus a 0.300 batting average with 5 home runs who steals at a 80% success rate? I envision a user having a collection of teams they have already experimented with that they will choose from to do a new experiment. They can always delete teams they no longer want to keep and they can add any teams they want. It would be similar to my browser favorites. I can add or delete favorites but every time I open the browser I start with the same list of favorites I ended with the last time I closed the browser.

[Kendall Ponder]

If you want to run it in its present state all of the code is at Baseball Simulation on GitHub. Most of the menus just return a print statement, but you can choose Modify a team and actually Modify the team description. Modify Batting order has a bug which I am trying to find. The TeamStorageFile has four teams in it already. It is used by the TeamStorage class to provide file storage for the teams. I haven't moved the Input class yet. I have shortened some of the lines and tried to clean up the indentation with Eclipse. I still need to look at the issue with the YesOrNo method and change the scan field to private.

[Campbell Ritchie]

The yes or no thing should be simple to cure.

[Kendall Ponder]

There is an inconsistency between the yes or no method and the invalid choice method.

I assume you are referring to the following code.

	public static String getYesOrNoFromTheUser(String promptMessage){
		System.out.format("%n%s%n", promptMessage);
		String userInput = getLowerCaseUserInput();
		if(userEnteredInvalidChoice(userInput)){
			System.out.format("%n%s%n", "You must enter Y, y, N or n.");
			userInput = getYesOrNoFromTheUser(promptMessage);
		}
		return userInput;
	}
	
	private static String getLowerCaseUserInput() {
		String userInput = scan.next();
		return userInput.toLowerCase();
	}
	
	private static boolean userEnteredInvalidChoice(String userInput) {
		return !(userInput.equals("y") || userInput.equals("n"));
	}

The code works. It will only accept Y, y, N or n so I'm not clear what the inconsistency is. The logic is

Change the input to lowercase
If the user entered an invalid choice ask for input again
and
The user entered an invalid choice if they did not enter y or n.

I need a hint to find what you found!

I found the bug in the Modify Batting Order code. I have made all the corrections except moving Input (I confess, I am using git from a terminal and Eclipse as my editor and am nervous about how to change the package and still have git track it but I am going to do it) and pushed them to the gitHub site linked to above. If you choose the second team you will be able to tell the Batting Order code works. That is assuming I pushed the correct TeamStorageFile to my gitHub account.

[Liutauras Vilda]

Hi Kendal,

I have one observation about the code below, actually about the middle method:

public static String getYesOrNoFromTheUser(String promptMessage){
    System.out.format("%n%s%n", promptMessage);
    String userInput = getLowerCaseUserInput();
    if(userEnteredInvalidChoice(userInput)){
        System.out.format("%n%s%n", "You must enter Y, y, N or n.");
        userInput = getYesOrNoFromTheUser(promptMessage);
    }
    return userInput;
}
 
private static String getLowerCaseUserInput() {
    String userInput = scan.next();
    return userInput.toLowerCase();
}
 
private static boolean userEnteredInvalidChoice(String userInput) {
    return !(userInput.equals("y") || userInput.equals("n"));
}

getLowerCaseUserInput() method seems to me redundant. String class has a nice method to achieve what you want by improving code in userEnteredInvalidChoise() method. Actual user input you suppose to take from user within getYesOrNoFromTheUser() method (this is what this method is meant to do according to the method name).
Check JavaAPI for String class, you should notice the method I have in mind.
Hint: it is related with "equals()" method of String class.

Hope it helps

[Liutauras Vilda]

Also, probably it is time to think, what happens and how you handle situations if user is not interested in entering Y, y, N or n, but instead, he is interested just hit "return" or anything else, for instance "4".

[Kendall Ponder]

Liutauras Vilda wrote:Hi Kendal,

I have one observation about the code below, actually about the middle method:

public static String getYesOrNoFromTheUser(String promptMessage){
    System.out.format("%n%s%n", promptMessage);
    String userInput = getLowerCaseUserInput();
    if(userEnteredInvalidChoice(userInput)){
        System.out.format("%n%s%n", "You must enter Y, y, N or n.");
        userInput = getYesOrNoFromTheUser(promptMessage);
    }
    return userInput;
}
 
private static String getLowerCaseUserInput() {
    String userInput = scan.next();
    return userInput.toLowerCase();
}
 
private static boolean userEnteredInvalidChoice(String userInput) {
    return !(userInput.equals("y") || userInput.equals("n"));
}

getLowerCaseUserInput() method seems to me redundant. String class has a nice method to achieve what you want by improving code in userEnteredInvalidChoise() method. Actual user input you suppose to take from user within getYesOrNoFromTheUser() method (this is what this method is meant to do according to the method name).
Check JavaAPI for String class, you should notice the method I have in mind.
Hint: it is related with "equals()" method of String class.

Hope it helps

Is this the simplification you were referring to?

public static String getYesOrNoFromTheUser(String promptMessage){
    System.out.format("%n%s%n", promptMessage);
    String userInput = scan.next();
    if(userEnteredInvalidChoice(userInput)){
        System.out.format("%n%s%n", "You must enter Y, y, N or n.");
        userInput = getYesOrNoFromTheUser(promptMessage);
    }
    return userInput;
}
 
private static boolean userEnteredInvalidChoice(String userInput) {
    return !(userInput.equalsIgnoreCase("y") || userInput.equalsIgnoreCase("n"));
}

I think it is better. Thanks!