• Post Reply Bookmark Topic Watch Topic
  • New Topic

Feedback on some basic code please  RSS feed

 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi everyone,

Would it be possible to get some feedback on some code I've just written? I've been working my way through Deitels' 'Java How to Program' for the past few months and thought I'd better ask for some help before my bad coding habits get too ingrained. The current problem I've just been working on is this:
(Airline Reservations System) A small airline has just purchased a computer for its new automated
reservations system. You have been asked to develop the new system. You are to write an application
to assign seats on each flight of the airline’s only plane (capacity: 10 seats).
Your application should display the following alternatives: Please type 1 for First Class and
Please type 2 for Economy. If the user types 1, your application should assign a seat in the first-class
section (seats 1–5). If the user types 2, your application should assign a seat in the economy section
(seats 6–10). Your application should then display a boarding pass indicating the person’s seat
number and whether it is in the first-class or economy section of the plane.
Use a one-dimensional array of primitive type boolean to represent the seating chart of the
plane. Initialize all the elements of the array to false to indicate that all the seats are empty. As
each seat is assigned, set the corresponding elements of the array to true to indicate that the seat is
no longer available.
Your application should never assign a seat that has already been assigned. When the economy
section is full, your application should ask the person if it is acceptable to be placed in the first-class
section (and vice versa). If yes, make the appropriate seat assignment. If no, display the message
"Next flight leaves in 3 hours."


The code I've come up with is this (please feel free to pick away):
 
Luigi Plinge
Ranch Hand
Posts: 441
IntelliJ IDE Scala Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's not bad. The biggest problem here is the code duplication between firstClassSeat() and economySeat(). If you find yourself copy-and-pasting, you're doing something wrong. you should combine these into one `bookSeat` method, taking a parameter to indicate class. You can use an `int` for the class of seat, but in real life it would be better to use an enumeration.

Your system for checking if each section is fully booked doesn't seem very watertight (although it might be OK in this toy example). You know it's fully booked if you've been through and not found a seat, not by checking if one particular seat is full. You could use a boolean flag, something like `seatAssigned` that starts as false, and gets set to true when you assign a seat. If it's still false at the end of the loop, you know the section was full. You can also use this flag in the for-loop condition, which allows you to avoid `break` (which is bad style, because it provides multiple exit points making code harder to follow). Better, you could break out the search into a separate method, taking two ints for the range, which allows you to re-use it. Something like

Have a go at fixing these. After you've done that, you could try separating your input/output (I/O) logic from the business logic. Your methods should only really do 1 thing each, and be just a few lines long at most. Here, you've mixed `println` methods in with logic for determining the outcome and assigning seats. Hint: have your methods return values based on what happened, rather than using `void`.
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Wow, thank you for that great reply Luigi. I really appreciate it.
I totally see what you mean about the duplicate code and agree that the boolean flag idea does seem sounder.
I had a feeling that the break statements were a bit sketchy too.
I'll have another go at it with the revisions you suggest and post back.
Thanks again.
 
Piet Souris
Master Rancher
Posts: 2044
75
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Let us know what you make of it after incorporating Luigi's remarks!

I would like to add two things:

1) in both methods "FirstClassSeat" and "EconomySeat" you call the method "start()" again, when someone is willing to accept a seat from another class. That gets you into a recursion with some unexpected outcomes!

2) your class does not have a "main" method. So you need some extra class or code to get it all going!

Success!
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Piet, will do.
Yes I wasn't too happy about using the start() method again so will look at some alternatives.
Oh yes, I have the main method in a separate class to get it all going. Will add that also next time.
Thanks!
 
Campbell Ritchie
Marshal
Posts: 56570
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nobody noticed a while (true) loop? Or == false?
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike Carroll wrote:Wow, thank you for that great reply Luigi. I really appreciate it.

And so you should.

I have to say, however, that you've been constrained by the question telling you how to implement your program.
If it was me, I think I'd create a Seat class. Then your "aircraft" or "flight" simply becomes a Seat[] (or, possibly even better, a Set<Seat>).
Then, you can incorporate all your "class" and "reserved" aspects (in fact, anything that has to do with a 'seat') in a single object.

That's a future enhancement though; maybe worth coming back to if you ever need to "improve" on this one.

Winston

PS: Is there any chance you could shorten the lines in your code? It makes your thread quite difficult to follow.
Read the DontWriteLongLines page for details.
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Winston,
I've shortened the comments that were carrying over to the next line now. Is that what you were referring to?
Apologies, I've only started posting today so am confidently clueless.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike Carroll wrote:Thanks Winston,
I've shortened the comments that were carrying over to the next line now. Is that what you were referring to?

Yup.

Basically, the rule of thumb is 80 characters max; and that includes comments or any long Strings you might need to initialize (break them up with the '+' operator).

Thanks

Winston
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Luigi Plinge wrote:It's not bad. The biggest problem here is the code duplication between firstClassSeat() and economySeat(). If you find yourself copy-and-pasting, you're doing something wrong. You should combine these into one `bookSeat` method, taking a parameter to indicate class. You can use an `int` for the class of seat, but in real life it would be better to use an enumeration.

Your system for checking if each section is fully booked doesn't seem very watertight (although it might be OK in this toy example). You know it's fully booked if you've been through and not found a seat, not by checking if one particular seat is full. You could use a boolean flag, something like `seatAssigned` that starts as false, and gets set to true when you assign a seat. If it's still false at the end of the loop, you know the section was full. You can also use this flag in the for-loop condition, which allows you to avoid `break` (which is bad style, because it provides multiple exit points making code harder to follow).


I've been having a go at making a bookSeat method using a parameter and also using a boolean flag to indicate when the section on the plane was full, but I'm finding myself getting nowhere fast. Do you think I would be better off starting from scratch as I'm finding in quite difficult to combine the above suggestions into the code that I've already written? Thanks.
 
Luigi Plinge
Ranch Hand
Posts: 441
IntelliJ IDE Scala Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'd start again if I were you. Shouldn't take too long since you can copy some of what you've already written. Just look at the places that are different in the two methods you have already, and replace with variables that you pass to the method: the upper and lower seat numbers, section name, etc. The question doesn't specify what to do if both sections are full, but you might want to add a boolean parameter to indicate whether the other section has already been checked.
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Okay, I'll have another crack at it over the weekend. Thanks!
 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's not bad. The biggest problem here is the code duplication between firstClassSeat() and economySeat(). If you find yourself copy-and-pasting, you're doing something wrong. You should combine these into one `bookSeat` method, taking a parameter to indicate class. You can use an `int` for the class of seat, but in real life it would be better to use an enumeration.

You could use a boolean flag, something like `seatAssigned` that starts as false, and gets set to true when you assign a seat. If it's still false at the end of the loop, you know the section was full. You can also use this flag in the for-loop condition, which allows you to avoid `break`

Thanks to Luigi's and everyone's suggestions I've managed to make some changes so far:
- Used an enumeration for the class of seat (haven't used enum's before so thanks for getting me to do that).
- Combined the firstClassSeat() and economySeat() methods into one bookSeat() method. (This is mostly based on Luigi's code, although I added the line 'seating[i] = true' (line 49) otherwise the method keeps assigning the same seat every time.
- Added the boolean flag 'seatAssigned' to break out of the loop (rather than using a break statement).

I've run into a problem though: I can't figure out how to not return seatNumber if the seats are full. In other words, the method still returns a value even if it's not been possible to assign a seat. Here's the revised code. Thank you for all your help.



 
Mark Channer
Greenhorn
Posts: 19
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If anyone has any bright ideas regarding the above, I'd appreciate whatever you have to say. It's driving me round the bend!
 
fred rosenberger
lowercase baba
Bartender
Posts: 12565
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
well...you defined the method as



that "int" part means that you HAVE to return an int, no matter what. You can't tell the compiler "this method returns an int", and then have the method sometimes return nothing...that's not allowed.

So rather than ask how to do something you can't, tell us what you want to do...what should happen if there is no seat? what should happen if there is a seat?
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!