Managing a cruise system

Follow up to Removing code smell from Room Booking code

This program organises cruises in which it sets up the ship with cabins/rooms. It also does reservations with customer information and the cost for available cabins/rooms.

This is my main class # program:

class Program
{
    static void Main(string[] args)
    {
        UBCruises bookings = new UBCruises ();
        Customer cust1 = new Customer ("Fred", "Johnstone", "A100356");
        Reservation booking = bookings.bookPassage("C", cust1, 2);       
        Console.WriteLine("Cust1's booking costs= {0}", booking.getCostofBooking());  //Astract Method from Reservation
        Console.Write ("Cabins booked are: ");
        ArrayList cabins = booking.cabins;
        for (int i = 0; i <  cabins.Count; i++)
            Console.Write ("{0}\t", ((room) cabins[i]).number);
        Console.WriteLine();
        Console.WriteLine();
        Customer cust2 = new Customer("Jane", "Jackson", "M892039");
        Reservation booking2 = bookings.bookPassage("H", cust2, 4);
        ///Console.WriteLine("Cust2's booking costs = {0}", booking2.cost);
        Console.WriteLine(booking2.getCostofBooking());
        Console.Write("Cabins booked are: ");
        cabins = booking2.cabins;
        for (int i = 0; i < cabins.Count; i++)
            Console.Write("{0}\t", ((room)cabins[i]).number);
        Console.WriteLine();
        Console.ReadLine();
    }
}

Customer class

public class Customer
{
    int discount = 1;
    String lastname, firstname;
    String accountNumber;

    public Customer(String firstname, String lastname, String accountNumber)
    {
        this.firstname = firstname;
        this.lastname = lastname;
        this.accountNumber = accountNumber;
    }
}

Reservation class

```
public class Reservation
{
public Customer booker;
public Boolean confirmed = true;
public int cost;
public ArrayList cabins = new ArrayList();

public Reservation(Customer booker, Boolean co

Customer Class

It's good practice to explicitly declare the scope of instance variables, if you don't specify it, they will public by default. However, instead of exposing instance variables as public, you should use properties with default Getter/Setters. private by default. Of course, me getting this confused is a good argument for explicitly declaring them.

public class Customer
{
    int discount = 1;
    String lastname, firstname;
    String accountNumber;

public int Discount {get; protected set;}
public string FirstName {get; protected set;}
public string LastName {get; protected set;}
public string AccountNumber {get; protected set;}

Note that once you've changed this, you can get rid of the this. syntax. It reduces clutter and repetitive keystrokes. We'll also move the initialization of discount to the constructor, which makes it more clear in my opinion. Also note that public properties and methods should be PascalCased, not lowercased.

public Customer(String firstname, String lastname, String accountNumber)
{
    FirstName = firstname;
    LastName = lastname;
    AccountNumber = accountNumber;
    Discount = 1;
}

Actually, I just now noticed that you never used the discount variable. Remove it instead.

Reservation class

More of the same advice, except....

public ArrayList cabins = new ArrayList();

ArrayList isn't type safe and its use is discouraged. This is because I could put any object I wanted to inside of that list. Instead, you should use a List.

private List cabins = new List();

Also, in the real world, a cost (or price) will rarely be a nice neat integer. Decimal is the proper datatype.

While I'm on the topic, I think that this should be named Price, not Cost. A cost is usually a cost to the business, whereas a price is what is charged to the customer. (Please disregard if I'm misreading your code.)

I like the flexibility the constructor overload gives, and I feel it reflects the very real possibilities of a person booking a single room, or many, but there some issues.

public Reservation(Customer booker, int cost, ArrayList cabins)
{
    this.booker = booker;
    this.cost = cost;
    this.cabins = cabins;
}

public Reservation(Customer booker, int cost, room theCabin)
{
    this.booker = booker;
    this.cost = cost;
    cabins.Add(theCabin);
}

First off, you're duplicating the code that sets this.booker and this.cost. Create a private method to set these.

private void SetBookerAndCost(Customer booker, int cost)
{
....
}

Or, alternatively, just forget the overload and only accept a List and force the client code to pass a list containing just one Room if there is only one room to book.

The second thing is that you seem to be missing that, unlike vb.net, C# is case sensitive. That means that an argument cabin is different from an argument Cabin. What I'm trying to get at is I don't feel like theCabin is a good name. Calling it just plain cabin would be 100% fine.

Room Class

More of the same; use properties rather than public variables and currency fields should have a decimal datatype.

I'm unsure of this though.

public String number;

At a glance, it's hard to tell why a field called number is being typed as a String. Then it hit me hard and clear. Of course it is. It's a room number, which you would not do math on, so string is indeed the right choice. Now, being it was not immediately obvious to me, consider changing it Name instead. Honestly, it's not a big deal, what you did is absolutely correct and any change to it would be a matter of opinion.

Ship Class

You need a private function here pretty badly. There is a lot of duplicated code where you've building the groups.

private List BuildGroup(decimal fare, string groupName, numberOfRooms)
{
    ArrayList group = new ArrayList();
    for (int i = 0; i < numberOfRooms; i++)
    {
        group.Add(new room(fare, groupName + (i + 1)));
    }
    return group;
}

Then your Ship class becomes more or less this.

...
List GroupA = BuildGroup(5000,"A",10);
List GroupB = BuildGroup(4000,"B",10);
...

UBCruises Class

Again, C# is case sensitive, so there's not reason to use ship1, just use ship.

Ship ship1;

I don't know if you can put this in a Switch statement, but if you can, you should.

if (cabinclass.Equals("a", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Balcony Suite");
    else if (cabinclass.Equals("b", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Suite");
    else if (cabinclass.Equals("c", StringComparison.OrdinalIgnoreCase))
        cabins = ship1.getDeck("Deck 3 - Outside Twin");

StackExchange Code Review Q#62432, answer score: 6