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?
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.12 Steak and Kidney pie
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 doWhat 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.
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.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. . . .
Isn't it!Kendall Ponder wrote: . . . this is much better! . . .
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.
If you print an error message use System.err not System.out.
No. If you ever need to print mainMenu, then you should override toString in both.Kendall Ponder wrote: . . .
So I should override toString for MainMenu class, not for the enum, correct?
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.. . . I think you mean the printMenu() method . . .
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.. . . isInvalid tells me if the input is invalid . . .
Yes. Unless you have redirected System.err.. . . If I send the message to System.err will they see it on the console?
Thank youAs always I appreciate the time you are giving me. I hope you have a great Father's Day weekend!
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.
Campbell Ritchie wrote:Welcome to JavaRanch
![]()
Rob Spoor points out it is completely possible to avoid the InputMismatchException like this, or something rather similar:
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.
But -1 is an integer. That is the problem with returning sentinel values.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 . . . .
Except possibly if you manage to pass max less than minA few minutes ago, I wrote: . . . Absolutely guaranteed to work nicely. . . .
Campbell Ritchie wrote:
But -1 is an integer. That is the problem with returning sentinel values.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 . . . .
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.
yesyesyesyesyes … and in a few weeks you can enhance it and the enhancements will be available for you to use for everKendall Ponder wrote: . . . put it in a Utility class and then I can use it for the rest of my life . . . Thanks!
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.
A bit more practice and you can be as objectionable as me.Kendall Ponder wrote: . . . before I knew anything about Clean Code or OO coding. . . .
Can't think of one at the moment.Sounds good to me, is there a disadvantage to making it static?
Don't know. Please supply the page number and I'll have a look in my copy.. . . in Effective Java, Bloch says to never derive a value associated with an enum with its ordinal. Does that not apply here . . .?
No, if you choose 3 twice in the same execution you get the same ModifyTeam object. I still think that will behave correctly.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 . . .
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. . . but isn't changing getIntFromMinToMax to an overloaded getInt less clear? . . .
You're welcome.. . . 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. . . .
Campbell Ritchie wrote:Is that Effective Java™ page 158?
There Bloch shows an enum like thisHe 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.
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.
No, if you choose 3 twice in the same execution you get the same ModifyTeam object. I still think that will behave correctly.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 . . .
Campbell Ritchie wrote:
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. . . but isn't changing getIntFromMinToMax to an overloaded getInt less clear? . . .
![]()
Remember that users of the method will often not be able to see the code.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 ahissy fitException.
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?
Liutauras Vilda wrote:Hi Kendal,
I have one observation about the code below, actually about the middle method:
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
Consider Paul's rocket mass heater. |