Feedback on some basic code please

[Mark Channer]

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):

import java.util.Scanner;
public class Airline 
{
	boolean[] seating = new boolean[11]; /* create 10 seat numbers. Empty seat indicated by false*/
	Scanner input = new Scanner(System.in);
	
	public void start()
	{		
		while ( true )
		{
			makeReservation();
		}	
	}
	
	public void makeReservation()
	{
		System.out.println("Please type 1 for First Class or 2 for Economy: ");
		int section = input.nextInt();
		if ( section == 1 )
		{
			firstClassSeat();
		}
		else
		{
			economySeat();
		}
	}
	
	public void firstClassSeat() // assign a first class seat
	{
		for ( int count = 1; count <= 5; count++ )
		{
			if ( seating[count] == false )  // if false, then a seat is available for assignment
			{
				seating[count] = true;	// assign seat
				System.out.printf("First Class. Seat# %d\n", count);
				break;
			}
			else if ( seating[5] == true ) // If seating[5] is true then first class is fully booked
			{
				if ( seating[10] == true) // If seating[10] is true then economy (and whole flight) is fully booked
				{
					System.out.println("Sorry, flight fully booked. Next flight is in 3 hours.");
				}
				else // ask passenger if they would like an economy ticket instead
				{
					System.out.println("First Class is fully booked. Would you like Economy? 1 for Yes 2 for No");
					int choice = input.nextInt();
					if ( choice == 1 )
					{
						economySeat();
						start();
					}
					else
					{
						System.out.println("Next flight is in 3 hours.");
						System.exit(0);
					}
				}
			}
		}
	}	
	
	public void economySeat() // assign an economy seat
	{
		for ( int count = 6; count <= 10; count++ )
		{
			if ( seating[count] == false ) // if false, then a seat is available for assignment
			{
				seating[count] = true; // assign seat
				System.out.printf("Economy. Seat# %d\n", count);
				break;
			}
			else if ( seating[10] == true ) // If seating[10] is true then economy is fully booked
			{
				if ( seating[5] == true) // If seating[5] is true then first class is fully booked
				{
					System.out.println("Sorry, flight fully booked. Next flight is in 3 hours.");
					System.exit(0);
				}
				else // ask if passenger would like a first class ticket instead
				{
					System.out.println("Economy is fully booked. Would you like First Class? 1 for Yes 2 for No");
					int choice = input.nextInt();
					if ( choice == 1 )
					{
						firstClassSeat();
						start();
					}
					else
					{
						System.out.println("Next flight is in 3 hours");
						System.exit(0);
					}
				}
			}
		}
	}
}

[Luigi Plinge]

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

    int findSeat(int lower, int upper) {
        int seatNumber = -1;
        for (int i = lower; seatNumber == -1 && i <= upper; i++) {
            if (seating[i] == false)
                seatNumber = i;
            }
        return seatNumber;
    }

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]

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]

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]

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]

Nobody noticed a while (true) loop? Or == false?

[Winston Gutkowski]

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]

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]

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]

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]

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]

Okay, I'll have another crack at it over the weekend. Thanks!

[Mark Channer]

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.

	import java.util.Scanner;
    public class RevisedAirline   
    {  
    	enum Seat{FIRSTCLASS, ECONOMY};
    	Seat seatClass;    	
        boolean[] seating = new boolean[11]; //create 10 seats. Empty seat indicated by false
        int upper; 
        int lower;
        
        Scanner input = new Scanner(System.in);  
          
        public void makeReservation()  
        {             	        	
            while (true) //Sorry, I'm still using this loop for now
            {  
                bookClass();  
                System.out.println(seatClass == Seat.FIRSTCLASS ? "First Class " : "Economy");
                System.out.printf("Seat# %d\n", bookSeat(lower, upper));  
            }     
        }  
        
        public void bookClass()  
        {  
            int section = 0;
            System.out.print("Please type 1 for First Class or 2 for Economy: ");  
            section = input.nextInt();  
            if (section == 1)  
            {  
            	seatClass = Seat.FIRSTCLASS;
            	lower = 0;
            	upper = 4;                
            }  
            else if (section == 2) 
            {  
            	seatClass = Seat.ECONOMY;
                lower = 5;
                upper = 9;
            }           
        }    
        
        public int bookSeat(int low, int upp) //assign a seat   
        {  
        	boolean seatAssigned = false; // flag
        	int seatNumber = -1;
            for (int i = low; seatAssigned == false && i <= upp; i++)  
            {  
                if (seating[i] == false)  //if false, then a seat is available for assignment  
                {
                	seating[i] = true; //set to true to mark seat as unavailable next time
                	seatNumber = i + 1;  
                	seatAssigned = true; //set flag to true to exit for loop
                }  
            }
            if (seatAssigned == false)
            {
            	System.out.println("Class full");
            }
            return seatNumber; 
         }
    }     
    

[Mark Channer]

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]

well...you defined the method as

public int bookSeat

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?