Removing code smell from Room Booking code

Question

I am new with coding and I recently read an article about code refactoring, so I have made a console application for booking rooms on a ship. I need help with code refactoring. As far as I understand the refactoring, I think in my project there are only 2 parts where I need refactoring, which are as follows.

One is a block of for statements:

 ship1 = new Ship("Olympic Countess");
 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)));
 }
 ship1.addDeck("Balcony Suite", groupA);
 ship1.addDeck("Suite", groupB);
 ship1.addDeck("Deck 3 - Outside Twin", groupC);
 ship1.addDeck("Deck 2 - Outside Twin", groupD);
 ship1.addDeck("Deck 1 - Outside Twin", groupE);
 ship1.addDeck("Deck 3 - Inside Twin", groupF);
 ship1.addDeck("Deck 2 - Inside Twin", groupG);
 ship1.addDeck("Deck 1 - Inside Twin", groupH); 
}

and the other one is the if else statement as follows:

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");

What I don't understand is that my parameters are changing in both logic, so how can I make a separate method for this logic when my cabin class is changing every time?

• Vogel612: I recommend you post a follow-up question to this after a few days and reviews containing your full code. Your complete approach seems a little "off".

Answer 1

What you are missing here is an enum-structure.

Enums are the tool of choice for predefined finite collections, that are typesafe.

public enum CabinClass 
{
 ClassA, ClassB, ClassC, //...
}

You could then Change your cabinclass from a String to a CabinClass.

Then that whole if-else construct becomes a trivial Switch-Case block:

public Reservation BookPassage (CabinClass cabinClass, Customer booker, int number)
{
 ArrayList cabins;
 switch (cabinClass) 
 {
 case CabinClass.ClassA:
 cabins = ship1.GetDeck("Balcony Suite");
 break;
 case CabinClass.ClassB:
 cabins = ship1.GetDeck("Suite");
 break;
 // ...
 }
 // whatever else you do here.
}

There's a few issues aside from this "design-flaw". Especially that's Naming:

The convention in c# is to name methods with PascalCase, private fields, parameters and variables in camelCase. This introduces a few changes, that I already did in the code I posted.

Additionally you should definitely look into Properties. They are one of the core features in C#. They allow code like this:

ship1.Decks["Balcony Suite"].Rooms;

The rest is hard to review, because there's still much context missing.

Answer 2

None of this information should be written into your program's source code. Rather, it should be stored in a configuration file, perhaps in XML, JSON, or YAML format — or maybe stored in a database instead. Not only would your code be less repetitive, it would also be more flexible, in that you wouldn't have to rebuild the application just to change trivial things such as product offerings and prices.

• Wayne Conrad: Outstanding. Most of the other answers are better ways to do the wrong thing; this answer is doing the right thing.
• Vogel612: @WayneConrad while what you write here is completely true. A full fledged configuratively working application seems to be above OP's current level.

Answer 3

ArrayList has been superseded by List<T> since .NET 2.0.

See all this repetition?

ArrayList groupA = new ArrayList();
for (int i = 0; i < 10; i++)
{
 groupA.Add(new room(5000, "A" + (i + 1)));
}

Let's replace that with a method:

private static List<Room> CreateDeck(int rooms, char name, int something)
{
 var deck = new List<Room>(rooms);
 for (var i = 0; i < rooms; i++)
 {
 deck.Add(new Room(something, string.Format("{0}{1}", name, i)));
 }
 return deck;
}

I don't know what the magic numbers (5000, etc) represent, so I just called that parameter something.

Please take @Vogel612's advice and post a follow-up question.

• Vogel612: I would guess that the magic numbers are prices, but I can't tell.

Answer 4

Extract duplicated code to a helper method

The duplication in the way you create groupA, groupB, ..., groupH:

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)));
}

... is really crying for a helper method, something like this:

private List<Room> CreateDeck(int price, String label, int num) {
 List<Room> group = new List<Room>();
 for (int i = 0; i < num; ++i) {
 group.Add(new Room(price, label + (i + 1)));
 }
 return group;
}

So that you can simplify your calls like this:

List<Room> groupA = CreateDeck(5000, "A", 10);
List<Room> groupB = CreateDeck(4000, "B", 10);
List<Room> groupC = CreateDeck(3500, "C", 30);
List<Room> groupD = CreateDeck(3400, "D", 36);
List<Room> groupE = CreateDeck(3300, "E", 40);
// ... and so on

Frankly, if a similar operation is duplicated twice, I would already create a helper function to avoid duplication. In your case the duplication is 8 times, for A, B, ..., H, so it's an absolute must.

Correctness

You named your class to represent a room as room, lowercase letters. The convention is to use CamelCase, so Room.

Convert if-else to a Map

In your code ship1 seems to have a mapping of deck names to lists of rooms, for example:

• "Balcony Suite" -> list of class A rooms
• "Suite" -> list of class B rooms
• "Deck 3 - Outside Twin" -> list of class C rooms

But then in BookPassage this map is not much use, the if-else statements basically emulate a labels -> list of rooms mapping, for example:

• "a" -> list of class A rooms
• "b" -> list of class B rooms
• "c" -> list of class C rooms

You did not share the source code of Ship, if possible, it would be good to extend that class to add another mapping inside it to simplify lookups. Change the method to make this work:

ship1.AddDeck("A", "Balcony Suite", groupA);
ship1.AddDeck("B", "Suite", groupB);
ship1.AddDeck("C", "Deck 3 - Outside Twin", groupC);
// ...

That is, add the label of the class, and internally build a Map to map the class names to the lists of rooms. This way your BookPassage class could become simply:

public Reservation BookPassage(String cabinclass, Customer booker, int number) {
 List<Room> cabins = ship1.GetDeckByCabinClass(cabinClass.ToUpper());
 // ... (no more if-else)
}

Answer 5

I don't have the whole scope of the code, but from what I can see i'm not sure if only refactoring is enough to get cleaner code. If you want to add some features it will require changes on several places.

Perhaps you should try to re-design your implementation. One example is to create an interface which handles if a passenger can book or not.

public interface ShipBookingFactory{
 boolean canBook(CabinClass cabinClass);
 Reservation bookCabin(CabinClass cabinClass);
}

Example above is using the Enum implementation of Vogel612 answer. By then you can easy add another class.