Problem
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 confirmed)
{
this.booker = booker;
this.confirmed = confirmed;
}
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);
}
public int getCostofBooking ()
{
return this.cost;
}
}
Room class
public class room
{
public int fare;
public String number;
public Boolean booked = false;
public room(int fare, String number)
{
this.fare = fare;
this.number = number;
}
}
Ship class
public class Ship
{
Dictionary<String, ArrayList> cabins = new Dictionary<String, ArrayList>();
String name;
public Ship(String name)
{
this.name = name;
this.setupShip();
}
public void setupShip() // this method must belongs to SHip class.
{
ArrayList groupA = new ArrayList();
for (int i = 0; i < 10; i++)
{
groupA.Add(new room(5000, "A" + (i + 1)));
}
ArrayList groupB = new ArrayList();
for (int i = 0; i < 10; i++)
{
groupB.Add(new room(4000, "B" + (i + 1)));
}
ArrayList groupC = new ArrayList();
for (int i = 0; i < 30; i++)
{
groupC.Add(new room(3500, "C" + (i + 1)));
}
ArrayList groupD = new ArrayList();
for (int i = 0; i < 36; i++)
{
groupD.Add(new room(3400, "D" + (i + 1)));
}
ArrayList groupE = new ArrayList();
for (int i = 0; i < 40; i++)
{
groupE.Add(new room(3300, "E" + (i + 1)));
}
ArrayList groupF = new ArrayList();
for (int i = 0; i < 30; i++)
{
groupF.Add(new room(3400, "F" + (i + 1)));
}
ArrayList groupG = new ArrayList();
for (int i = 0; i < 36; i++)
{
groupG.Add(new room(3300, "G" + (i + 1)));
}
ArrayList groupH = new ArrayList();
for (int i = 0; i < 40; i++)
{
groupH.Add(new room(3200, "H" + (i + 1)));
}
addDeck("Balcony Suite", groupA);
addDeck("Suite", groupB);
addDeck("Deck 3 - Outside Twin", groupC);
addDeck("Deck 2 - Outside Twin", groupD);
addDeck("Deck 1 - Outside Twin", groupE);
addDeck("Deck 3 - Inside Twin", groupF);
addDeck("Deck 2 - Inside Twin", groupG);
addDeck("Deck 1 - Inside Twin", groupH);
}
public void addDeck(String deckName, ArrayList cabins)
{
this.cabins.Add(deckName, cabins);
}
public ArrayList getDeck(String deckName)
{
return (ArrayList)cabins[deckName];
}
}
UBCruises class
public class UBCruises
{
Ship ship1;
public UBCruises()
{
ship1 = new Ship("Olympic Countess");
}
// Create instance of the ship and call the setupShip() from ship constructor.
// setupShip() method
public Reservation bookPassage(String cabinclass, Customer booker, int number)
{
ArrayList cabins;
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");
else if (cabinclass.Equals("d", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 2 - Outside Twin");
else if (cabinclass.Equals("e", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 1 - Outside Twin");
else if (cabinclass.Equals("f", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 3 - Inside Twin");
else if (cabinclass.Equals("g", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 2 - Inside Twin");
else
cabins = ship1.getDeck("Deck 1 - Inside Twin");
if (available(cabins, number))
{
ArrayList bookedcabins = new ArrayList();
int currentCabin = 0;
int cost = 0;
while (number > 0)
{
room thisCabin = (room)cabins[currentCabin];
if (!thisCabin.booked)
{
bookedcabins.Add(thisCabin);
cost += thisCabin.fare;
number--;
}
currentCabin++;
}
return new Reservation(booker, cost, bookedcabins);
}
else
return new Reservation(booker, false);
}
public Boolean available(ArrayList cabins, int number)
{
int freeCount = 0;
for (int i = 0; i < cabins.Count; i++)
{
if (!((room)cabins[i]).booked)
freeCount++;
}
return number <= freeCount;
}
}
}
Solution
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<Room> cabins = new List<Room>();
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<Room> 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<Room> 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<Room> GroupA = BuildGroup(5000,"A",10);
List<Room> 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");
First Thoughts
Indentation
The indentation on the classes is a bit strange. it is consistent, so I assume that it is intended but I have not seen used much (at all) elsewhere. Unless there is a very strong reason it is usually best to go with the majority on such things.
Collections
Is there are reason for using ArrayList that than a List<Room>? There seems to be a lot of drawbacks with no advantages.
Public Fields
As a general rule, one should not have fields in the public interface of a class. A good example is cabins in the Reservation class. It is initialized to a new ArrayList() but as it is public there is no safety net preventing it being overwritten clearing out any previously booked rooms.
Intended usage
Following on from above, even making them into properties doesn’t solve the problem if we merely make them public get; set; properties. If the intent for a reservation to be for a given customer then the Customer should be a read property set only on construction.
Chained Constructors
Possibly more a matter of style than a hard review point but I would tend to chain multiple constructors to a single one that is responsible for setting the fields rather than having multiple ctors setting the fields. This makes it less likely than if we need to change the class that some ctors get updated and others to not leaving the created class in an invalid state.
public class Reservation {
public Reservation(Customer booker, int cost, Room theCabin)
: Reservation (booker, cost, new []{theCabin}, false) {}
public Reservation(Customer booker, bool confirmed)
: Reservation (booker, 0, new []{}, confirmed) {}
public Reservation(Customer booker, int cost, IEnumerable<Room> theCabins, bool confirmed = false) {
Booker = booker;
Cost = cost;
Cabins = theCabins.ToList();
}
public Customer Booker { get; private set;}
public int Cost { get; private set;}
public IEumerable<Room> Cabins { get; private set;}
}
System vs. C# types
I cannot find an authoritative source but AFAIK, the recommendation is to use C# types (int, string, bool, float et al.) as opposed to the System types (Integer, String, Boolean, Double). I have no strong reason why it should be so but I mention it because we have a mixture in the code String instead of string but int used not Integer. Whichever set is used, we should be consistent.
Naming
Class names should start with a capital Room not room, as should method names e.g getCostOfBooking() and setupShip(). Note: SetupShip should be private. There is no need for the outside world to know about it.
Separation of responsibilities
A Ship should really only be a container for rooms. The initialization can be performed by some other class (call it, say, a ShipConfiguration) that it responsible for defining the rooms on each deck. This allows us to easily have different ship layouts.
The hardcoded names for the cabin classes and decks is very brittle. The mapping of cabin classes to available cabins should be part of the configuration for the ship not part of BookPassage. Pass a cabinclass into a Ship and it should be able to tell you how may cabins are available of that type. As it stands, adding another ship with different classes and decks will be very messy. Depending upon how one wants to structure it, the booking could happen as a single operation. Book n cabins of class x on the ship and the returns an enumerable of the cabins or an empty enumerable if not enough are available. It can be tweaked in a number of ways depending upon exact usage requirements but the import point is to encapsulate the variable piece – mapping the cabin class to the ‘deck’ and letting it be different for each ship.
Where I’m Going
- Define data structures and model the “cruses” domain in a more OO way
- “Push details down” into appropriate classes. aka Encapsulation.
- See what the resulting structures and encapsulation do for the other classes.
- Code volume in many classes will shrink dramatically
- Code will start to read as what it’s doing.
- Fewer bugs
enum uber string
public enum CabinClass {
undefined,
A, B, C, //etc.
}
public enum Deck {
undefined,
BalconySuite,
Suite,
}
I expect lots of code will simplify and be less error prone. I love enum type safety and visual studio intellisense. Further, “undefined” is great for default values. It forces explicit setting in code – If it defaults to “A”, for example, how can you tell that’s wrong?
Define CabinClass and Deck relationship
public Dictionary<CabinClass, Deck> CabinDeckMap = new Dictionary<CabinClass, Deck> {
CabinClass.undefined, Deck.undefined,
CablinClass.A, Deck.BalconySuite,
CablinClass.B, Deck.Suite,
// etc.
}
- is
CabinClassexactly the same thing asGroup? Looks like it.
Cabin Collection – define the cabins on a ship
public CabinCollection : List<Cabin> {
// now we have a place to put any "cabin array list" functions.
public CabinCollection GetCabinsInClass(CabinClass thisClass) {
// this would be very short using LINQ
CabinCollection groupCollection = new CabinCollection();
foreach (Cabin cabin in this) {
if(cabin.Class == thisClass)
groupCollection.Add(cabin);
}
return groupCollection;
}
public CabinCollection GetUnbookedCabins(CabinClass thisClass) {
// this would be very short using LINQ
CabinCollection deckCollection = new CabinCollection();
foreach (Cabin cabin in this) {
if(cabin.Class == thisClass && !cabin.booked)
deckCollection.Add(cabin);
}
return deckCollection;
}
public CabinCollection GetCabinsOnDeck(Deck thisDeck) {
// this would be very short using LINQ
CabinCollection deckCollection = new CabinCollection();
foreach (Cabin cabin in this) {
if(cabin.Deck == thisDeck)
deckCollection.Add(cabin);
}
return deckCollection;
public override string ToString() {
StringBuilder me = new StringBuilder(); // System.IO namespace
foreach( Cabin cabin in this) {
me.AppendLine(cabin); // ToString is implicitly called
}
return me.ToString();
}
} // end class
room class group is fuzzy
- Room names (numbers) and “group” are redundant. The room number prefix IS the group.
- If “group” and “deck” relation ship is used throughout then define another
enum SetupShip()will simplify because we don’t need a ArrayList for each group
I think “Group” is irrelevant. CabinClass is what it’s about.
public class Cabin {
public string Name {
get {return Class.ToString() + RoomNumber;}
// no set property
}
protected string RoomNumber { get; set; }
public CabinClass Class {get; protected set;}
public Deck deck {get; protected set;}
public Cabin ( double fare, CabinClass cabinClass, int number ){
this.fare = fare;
RoomNumber = number.ToString();
this.Class = cabinClass;
CabinDeckMap.TryGetValue(this.Class, out this.deck);
}
public override string ToString() {
return string.Format("Name: {0}, Class: {1}, Deck: {2}", this.Name, this.Class, this.deck);
}
}
- passing an
intfor number forces user to give us digits - Cabin.Name looks like a string to clients and they don’t have to deal with the distinction between “group” and “number”.
Effects in client code
ArrayList cabins = booking.cabins;
for (int i = 0; i < cabins.Count; i++)
Console.Write ("{0}t", ((room) cabins[i]).number);
// becomes
Console.Write(bookings2.Cabins);
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");
else if (cabinclass.Equals("d", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 2 - Outside Twin");
else if (cabinclass.Equals("e", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 1 - Outside Twin");
else if (cabinclass.Equals("f", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 3 - Inside Twin");
else if (cabinclass.Equals("g", StringComparison.OrdinalIgnoreCase))
cabins = ship1.getDeck("Deck 2 - Inside Twin");
else
cabins = ship1.getDeck("Deck 1 - Inside Twin");
// All above goes away because CabinDeckMap does this for us and
// the Cabin constructor get the deck value.
// Then CabinCollection makes client code readable and short as seen here
public Reservation bookPassage(CabinClass cabinclass, Customer booker, int number) {
// "cabinclass" is not a string so it is impossible (almost) to pass in anything
// not defined in the CabinClass enum.
CabinCollection availableCabins = ship1.Cabins.GetUnbookedCabins(cabinClass);
if(availableCabins.Count > 0)
BookACabin(availableCabins, booker, number);
Ships have Cabins – not “array of strings” and not ” cabins and/or rooms”
public class Ship {
public CabinCollection Cabins {get; protected set;}
protected SetupShip() {
for (int i = 0; i < 10; i++) {
Cabins.Add(new Cabin(5000, CabinClass.A, i));
Cabins.Add(new Cabin(4000, CabinClass.B, i));
// ...
Cabins.Add(new Cabin(1000, CabinClass.H, i));
}
}
// if we created a CabinClass to price Dictionary then
// this class would not have to know that. And
// the Cabin constructor would look it up and so the
// above would look like this:
Cabins.Add(new Cabin(CabinClass.A, i));
}
- 8 ArrayLists are not created
- 43-ish physical lines removed
- Deck, Name, Price are all encapsulated in the
Cabinclass – where they belong; so now client code tends to be written, read, and understood in terms ofCabinClassalone.
EDIT – A better Cabin
take Cabin encapsulation farther, and better, into OO-ness…
Have CabinCollection class create it’s own room numbers.
This will fix a bug in existing code. Then creating cabins will look like this:
for (int i = 1; i <= 10; i++) {
Cabins.Add(new Cabin(CabinClass.A));
}
- It makes sense that the
CabinCollectionshould assign numbers because it knows, it HAS, all the cabins. And it can avoid multiple cabins with the same room number - So we could say that
CabinCollectionis the “lowest” or “first” place in our domain model where we have the context (the set of all cabins) that allows us to generate unique cabin numbers. This is pushing details down - Then the
forloop is just creating N cabins. You could say this is separation of concerns in miniature.
The bug
Current code can create many cabins with the same room number
for (int i = 1; i <= 10; i++) {
Cabins.Add(new Cabin(CabinClass.A, i));
}
// meanwhile, somewhere else... this may or may not be in the same class
for (int i = 1; i <= 10; i++) {
Cabins.Add(new Cabin(CabinClass.A, i));
}
- We now have a bunch of cabins with the same names
- Put the room numbering in
CabinCollection.Add() - Wherever
CabinCollectionis used, room number logic goes with it – This is reusable code
A few thoughts looking at your code:
Customer class:
You should probably consider including contact information for the customer, since realistically a booking agent would need to get a hold of the customer after the booking is made.
Can the discount be any amount or will you want set amounts? If the latter, a Dictionary to hold the values would probably be easier to maintain and gives you the option to give the discounts a descriptive name and edit which ones you want to allow.
Reservation class:
It would make sense to include dates and a descriptive name for the cruise that the reservation is for. This puts the information you need in one place.
Ship class:
Since you have a lot of code assigning rooms to decks, a Deck class to hold the rooms and a collection of decks that belongs to the ship, would go a long way toward cleaning up that part of your code.