• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Ron McLeod
  • Junilu Lacar
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • Jeanne Boyarsky
  • Rob Spoor
  • Bear Bibeault
Saloon Keepers:
  • Tim Moores
  • Tim Holloway
  • Piet Souris
  • Carey Brown
  • Stephan van Hulst
Bartenders:
  • Frits Walraven
  • fred rosenberger
  • salvin francis

Menu problem

 
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi, having a problem with this program whereby if you select option 1 and  enter in the numbers to be calculated it will calculate them but once you exit the loop by entering -1 if you select option 1 again it won't let you back in to do another calculation, any ideas?



I plan on filling out the option 2 and 3 methods later on, there just tester to make sure they could be called.
 
Greenhorn
Posts: 9
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Could it have something to do with the exit variable on lines 67 and 110? On line 67 you are checking if it's false. Since you are probably setting it to true on line 110, the while loop won't get executed.

Let me know.
 
Saloon Keeper
Posts: 8283
71
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
"number" should be a local variable and initialized to true. Do not use a member variable for this.
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carey Brown wrote:"number" should be a local variable and initialized to true. Do not use a member variable for this.



You legend that sorted it, thank you!
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Marc Greenwood wrote:Could it have something to do with the exit variable on lines 67 and 110? On line 67 you are checking if it's false. Since you are probably setting it to true on line 110, the while loop won't get executed.

Let me know.



Thanks for the suggestion, carey's solution sorted it.
 
Marshal
Posts: 73281
332
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Yur indentation and formatting are inconsistent, which makes your code hard to read. And If I have difficulty reading it, rest assured, you will ind it even more difficult! This is what I think it should look like, though I haven't got the time to correct every error. If you use }else{ all on one line, which I don't like myself, put spaces around the braces. Make sure there are empty lines between successive methods.Most of your code uses K&R indentation but a few parts use Allman indentation. Don't mix conventions. Some of your variable names, e.g. number, are confusing. Don't combine Scanner#nextLine and Integer#parseInt. Use Scanner#nextInt, but as explained here, you may have to call nextLine twice. Don't write if (b) return true; else return false; As explained here, you shoiuld simply write return b;
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Cheers Campbell, always good to get feedback.
 
Campbell Ritchie
Marshal
Posts: 73281
332
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
That's a pleasure  

There's lots more.
You probably don't need the keepGoing local variable; you can replace it with the count and < or similar.
Your class name isn't good; it shoiuld be obvious from the class name what it represents.
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Damn and there was me thinking I had fairly good Java convention :p
 
Campbell Ritchie
Marshal
Posts: 73281
332
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The problem with not following conventions is that you can miss more serious errors. You have already been told that the variable number shouldn't be a field; I can see a potential logic error arising from that.  I shall leave you to work out what it is. Maybe the baleful effects of that error would only have a theoretical existence, but you should still know about it.
 
Marshal
Posts: 16451
272
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Many of the names chosen are misleading. For example, why would you choose a name like number for a boolean value? isNumber makes more sense as a boolean. The "is" part of the name makes all the difference.

A method name should indicate an action and hence, should be a verb or a verb phrase. passwordChecker is a noun phrase. It's a thing. That kind of name is best used for classes, interfaces, enums, and variables (except boolean). That method name would have been better as checkPassword(), except that the method doesn't only check the password. It does other things as well. That's a problem with the implementation though. If your method name declares that only the password is being checked, then there should be no code inside its body that does other things besides checking the password. Doing other things only makes the method name lie about its purpose.
 
Campbell Ritchie
Marshal
Posts: 73281
332
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:. . . why would you choose a name like number for a boolean value? isNumber makes more sense as a boolean. . . . .

I would have gone for something like needsInput or needsNumberInput. You can read more about name conventions here; that is an old resource but still valid. You will find that the legibility of if‑s and loop can be greatly enhanced by judicious choice of variable names, and also by choosing something not needing a not operator (!). Read the loop aloud, preferably while nobody is listening, and that should make it clear whether you have a good name for your boolean variable or not.
I would consider a loop like this:-I shall leave you to find out how to avoid the duplicated code in lines 1 and 5. But the boolean variable has disappeared. Remember, every variable you have constitutes an opportunity for something to go wrong
 
Junilu Lacar
Marshal
Posts: 16451
272
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Junilu Lacar wrote:. . . why would you choose a name like number for a boolean value? isNumber makes more sense as a boolean. . . . .

I would have gone for something like needsInput or needsNumberInput.


Maybe. I was speaking in general terms, not specifically about the context that OP used that particular name in. Looking at the getOption1() method, however, I see that OP has again chosen a misleading and unexpressive name. That method is doing so much more than "getting Option 1." In the first place, you have to search the surrounding code to figure out what "get Option 1" means. Then, after reading the code inside that method, you see that the method is actually doing multiple things: Getting one number and validating it, getting another number and validating it, guarding against bad input and controlling flow of control around getting the numbers previously mentioned and validating them, and then, somewhere in that whole mess, printing out some message about the two numbers that were input and validated, after calling another method, getAnswer(), that processes those two numbers and returns what is presumably "the answer," whatever that means.

Wow, all that and the only thing that describes it all is the name "getOption1."  
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Many of the names chosen are misleading. For example, why would you choose a name like number for a boolean value? isNumber makes more sense as a boolean. The "is" part of the name makes all the difference.

A method name should indicate an action and hence, should be a verb or a verb phrase. passwordChecker is a noun phrase. It's a thing. That kind of name is best used for classes, interfaces, enums, and variables (except boolean). That method name would have been better as checkPassword(), except that the method doesn't only check the password. It does other things as well. That's a problem with the implementation though. If your method name declares that only the password is being checked, then there should be no code inside its body that does other things besides checking the password. Doing other things only makes the method name lie about its purpose.



The choice of names was temporary until I got a skeleton working, I was going to go back then and change everything.
 
Kevin O'Sullivan
Ranch Hand
Posts: 103
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

Campbell Ritchie wrote:

Junilu Lacar wrote:. . . why would you choose a name like number for a boolean value? isNumber makes more sense as a boolean. . . . .

I would have gone for something like needsInput or needsNumberInput.


Maybe. I was speaking in general terms, not specifically about the context that OP used that particular name in. Looking at the getOption1() method, however, I see that OP has again chosen a misleading and unexpressive name. That method is doing so much more than "getting Option 1." In the first place, you have to search the surrounding code to figure out what "get Option 1" means. Then, after reading the code inside that method, you see that the method is actually doing multiple things: Getting one number and validating it, getting another number and validating it, guarding against bad input and controlling flow of control around getting the numbers previously mentioned and validating them, and then, somewhere in that whole mess, printing out some message about the two numbers that were input and validated, after calling another method, getAnswer(), that processes those two numbers and returns what is presumably "the answer," whatever that means.

Wow, all that and the only thing that describes it all is the name "getOption1."  



Again temporary until I decided what I wanted that method to do and was going to name appropriately then once I had decided.
 
Campbell Ritchie
Marshal
Posts: 73281
332
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Please don't quote the whole of the old post; that simply makes the thread longer without adding content.

Kevin O'Sullivan wrote:. . . temporary . . .

I have heard that one before
Temporary names are simply another way for you to make a rod for your own back. Decide what the method is going to do before you write it, then you can give it a sensible name. Otherwise you will get compiler errors everywhere you have written option 1. If you start off with a switch statement looking like this:-. . . that is better than tying yourself in knots with temporary names.
 
Junilu Lacar
Marshal
Posts: 16451
272
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The funny thing about "temporary" code like that is it has superglue properties. It quickly sticks and hardens. There is a lot of production code out there with misleading "temporary" names like these.

Getting your code to work is fine. Getting 50 or 100 hundred lines of code to work before you start addressing code smells like unexpressive and downright misleading names is NOT fine. Names that don't express their intent and purpose well get in the way of any further work you do on the code. That includes testing, debugging, and refactoring. The easier your code is to understand and the sooner you make it like that, the faster you will go in the long run.

Good programmers tend to do this:
- Write tests that specify the behavior you want. See the test fail to confirm you don't have that behavior yet.
- Write the code to make the failing test pass.
- Refactor the code to make it clear, expressive, and simple. Make sure all tests still pass

This requires a lot of discipline and skill to do but if you start following this process, your life as a programmer will be much easier and productive and so will the lives of other people who have to read and work with your code.
 
Junilu Lacar
Marshal
Posts: 16451
272
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
To give you an example of how little effort should be involved in refactoring, imagine you just wrote the code in this method:

If you just make a habit of stopping for a minute to think about the design of this method, how things are named and how clearly the code expresses intent, you might think the following:

"Hmmm... does the name menuOption really express what this method is doing? Maybe not. What if I tried this instead?"

"Ok, maybe that's a little more expressive. Let's see, what else can I clarify...? Well, those method calls aren't that expressive either. What if I try this...?"

"Ok, that looks a little better. At least now, I won't have to remember all those option numbers and others can figure out quickly what the action is about instead of having to scroll around to look at the implementation."

You can then continue with a little bit clearer code to work with.
 
reply
    Bookmark Topic Watch Topic
  • New Topic