Algorithm movie ticket booking

Question

Just for fun and practice purpose, I have written a short program in C# console with few classes which will create 30 seats for a cinema during program initialization with these options.

• Print all seats
• Seed 20 random taken seats
• Print all empty seats
• Print all taken seats

I think my algorithm for generate 20 random taken seats doesn't look that efficient. I thought of extra space, which is the taken list. For that, while generated empty seats not equal with required empty seats, I will remove the random seat and push that random seat to taken list. But the problem with this is to print all the complete 30 seats, I have merge taken and empty list.

Anyway is my code for review.

 /// <summary>
/// One cinema will have many seats
/// </summary>
public class Cinema
{
 private readonly List<Seat> seats;
 public Cinema()
 {
 // Maximum number of seat of any cinema is 30.
 seats = new List<Seat>(30);
 GenerateCompleteSeats();
 }
 private void GenerateCompleteSeats()
 {
 int totalRow = 3;
 int totalCol = 10;
 for (int i = 0; i < totalRow; i++)
 {
 for (int y = 0; y < totalCol; y++)
 {
 seats.Add(new Seat(i+1, y+1));
 }
 }
 }
 private void PrintSeats(List<Seat> _seats)
 {
 foreach (var item in _seats)
 {
 Console.WriteLine($"Row: {item.Row}. Col: {item.Col}. Status: {item.Status}");
 }
 }
 internal void PrintCompleteSeats()
 {
 Console.WriteLine("Total seats: " + seats.Count);
 PrintSeats(seats);
 }
 internal void SeedCinemaSeats()
 {
 const int totalTakenSeatsNeeded = 20;
 int totalGeneratedEmptySeats = 0;
 // totalSeats will keep increasing until equal to total empty seats needed
 while (totalGeneratedEmptySeats != totalTakenSeatsNeeded)
 {
 // Generate a random number
 var randomNo = new Random();
 // Get a random seat
 var randomSeat = seats[randomNo.Next(seats.Count)];
 // Remove random seat
 //seats.Remove(randomSeat);
 // Update random seat status
 if(randomSeat.Status == Seat.EnumStatus.Empty)
 {
 randomSeat.Status = Seat.EnumStatus.Taken;
 totalGeneratedEmptySeats++;
 }
 }
 }
 internal void PrintEmptySeats()
 {
 var empty = seats.Where(s => s.Status == Seat.EnumStatus.Empty);
 Console.WriteLine("Total Seats: " + empty.Count());
 PrintSeats(empty.ToList());
 }
 internal void PrintTakenSeats()
 {
 var taken = seats.Where(s => s.Status == Seat.EnumStatus.Taken);
 Console.WriteLine("Total Seats: " + taken.Count());
 PrintSeats(taken.ToList());
 } 
}
public class Seat
{
 public int Row { get; }
 public int Col { get; }
 public EnumStatus Status { get; set; }
 /// <summary>
 /// A valid seat object will always need row and col.
 /// </summary>
 /// <param name="row"></param>
 /// <param name="col"></param>
 public Seat(int row, int col)
 {
 this.Row = row;
 this.Col = col;
 this.Status = EnumStatus.Empty;
 }
 public enum EnumStatus
 {
 Empty, Taken
 }
}
static void Main(string[] args)
 {
 // Initialize Cinema
 var cinema = new Cinema();
 while (true)
 {
 Console.Clear();
 Console.WriteLine("1. Print all Seats in Cinema");
 Console.WriteLine("2. Seed some sample booked seats");
 Console.WriteLine("3. Print empty/available seats");
 Console.WriteLine("4. Print taken seats");
 var opt = Console.ReadLine();
 switch (opt)
 {
 case "1":
 cinema.PrintCompleteSeats();
 break;
 case "2":
 cinema.SeedCinemaSeats();
 break;
 case "3":
 cinema.PrintEmptySeats();
 break;
 case "4":
 cinema.PrintTakenSeats();
 break;
 default:
 break;
 }
 Console.ReadKey();
 }
 }

• pacmaninbw: It would make it much easier for some users if you had the necessary using statements in the code. Did you write this as 1 file or 3 files?
• BCdotWEB: WRT var randomNo = new Random();: stackoverflow.com/a/768001/648075
• Steve Ngai: @pacmaninbw 3 files. So set as static for random. Thanks BCdotWEB

Answer 1

Picking random seats

I think my algorithm for generate 20 random taken seats doesn't look that efficient.

while (totalGeneratedEmptySeats != totalTakenSeatsNeeded)
{
 // Generate a random number
 var randomNo = new Random();
 // Get a random seat
 var randomSeat = seats[randomNo.Next(seats.Count)];
 // Remove random seat
 //seats.Remove(randomSeat);
 // Update random seat status
 if(randomSeat.Status == Seat.EnumStatus.Empty)
 {
 randomSeat.Status = Seat.EnumStatus.Taken;
 totalGeneratedEmptySeats++;
 }
}

You shouldn't recreate a new Random (= random number generator, not a number by itself) for every new number, you should be using the same Random and requesting numbers multiple times. To that effect, put your initialization outside of the loop:

var random = new Random();
while(...)
{
 // ...
}

The randomization process can also be optimized, as you're currently running into possible retries when you randomly select a seat you had already selected before. That's inefficient, and it can be avoided by changing your "shuffle and draw" approach.

Using the example of a deck of cards, if you want to draw 10 random cards, you don't need to draw these cards separately, shuffling the deck again each time (and putting the drawn card back in the deck on top of that). You can simply shuffle the deck and take the top 10 cards. Since the deck is in random order, the top 10 cards are as random as any other group of 10 cards would be.

This also avoids having to retry draws, as the top 10 cards of the deck are guaranteed to not overlap with one another.

Using LINQ, this can be done quite tersely:

var shuffledSeats = seats.OrderBy(seat => random.Next());

Usually, you pick an ordering method that related to the seat (e.g. OrderBy(seat => seat.Price)), but in this case, we tell LINQ to order it by a random number, which effectively means that LINQ will randomly order our list.

We then take the first 20 seats:

var twentyRandomSeats = shuffledSeats.Take(20);

and then we register these seats as taken:

foreach(var seat in twentyRandomSeats)
{
 seat.Status = Seat.EnumStatus.Taken;
}

These operations can be chained together:

foreach(var seat in seats.OrderBy(seat => random.Next()).Take(20))
{
 seat.Status = Seat.EnumStatus.Taken;
}

Whether you chain them or not is up to you. It's a readability argument. It's definitely not wrong to keep the steps separate if you find it clearer.

---

Separating taken seats from empty seats

I will remove the random seat and push that random seat to taken list. But the problem with this is to print all the complete 30 seats, I have merge taken and empty list.

This can indeed be an issue when you want to handle the complete list too. And you don't want to store three separate lists (all, taken, empty) as they may become desynchronized and it's a generally cumbersome juggling act.

Since each seat carries its own status which indicates whether it's taken or not, we can simple keep all seats together in a single list, and then filter that list when we need to.

LINQ allows for a terse and clean to read syntax:

var emptySeats = seats.Where(seat => seat.Status == Seat.EnumStatus.Empty);
var takenSeats = seats.Where(seat => seat.Status == Seat.EnumStatus.Taken);

Since your seats list is a class field, you can define the other lists as computed class fields:

class Cinema
{
 private readonly List<Seat> seats = new List<Seat>();
 private List<Seat> takenSeats => seats.Where(seat => seat.Status == Seat.EnumStatus.Taken);
 private List<Seat> emptySeats => seats.Where(seat => seat.Status == Seat.EnumStatus.Empty);
}

You can add null-checking here if you need it, but I would generally advise to avoid having nulls instead of continually having to check for it. To that effect, I've given seats a default value. As long as you don't explicitly make it null, you don't need to continually null check.

• Rick Davin: Seat could be a struct rather than a class.
• Flater: @RickDavin: Feel free to add an answer of your own. Code reviews aren't mutually exclusive, several answers can each add valuable information.
• Steve Ngai: @Flater. That short random algorithm using linq you gave is awesome. I did a simple blackjack app before for fun. But couldn't figure out that, it can be used in here also. Thanks a lot for great code review.

Answer 2

It seems you are a beginner / hobbyist / learner. This was not intended as an offense since we were all beginners once.

The biggest advice I could pass on is that you should separate the UI from the logic. Anything with displaying output or requesting input is part of that UI. The Cinema class should not be issuing Console.WriteLine. Rather it could instead return a string and leave it up to the UI on what to do with that string.

@Flater had good points all around, particularly about having the Random instance be defined at the class level. As for randomizing seats, I would treat all the seats like a deck of card. I would randomly shuffle a deck once, and then deal each card as requested. I would not randomly shuffle, deal one card, and then randomly shuffle again for the next card. I suggest you look into the Fisher Yates Shuffle (several examples are here on CR).

The Seat class could become an immutable struct instead. You may consider overriding ToString to print an "E" or "T" for empty or taken, or perhaps "O" and "X" for open and taken.

• Steve Ngai: Thanks for the code review and pointers. Yeah I haven't refactor my code in that posted version for the separation of concern because I just focus on that random algorithm for quick review. Anyway, now I have created 6 projects in that one solution to split them up as separation of concern purpose and make sure that all my Console.Writeline in my Console project. you mean Seat should be a struct instead of class?
• Steve Ngai: After I change the Seat from class to struct, it doesn't compile. Looks like after changed to struct, I can't modify its property.
• Rick Davin: Then keep Seat as a class. You should still override ToString.
• Steve Ngai: May I know the reason to override tostring? Because now I have modify it using extension method to format it to single letter. But data will save as enum

Answer 3

The menu should include an option to exit the program.

It might be nice if the menu refreshed after every option was completed.

The seats open or taken would be clearer if the theater was printed as a matrix, maybe use the letters E and T to show empty and taken seats.

For future reviews it would probably be better to show each file rather than merging them all together.

It's not clear why the this reference was used in the seat constructor.