6
\$\begingroup\$

I'm attempting to improve my OOP design skills through solving sample problems. Below is an implementation of a flight booking system where the passenger can only request a seat class, but no number. Also, flight statistics must be printed.

In your review, please focus on design: How can I design the classes better (or design better classes) to lower coupling, increase cohesion and maintainability?

One source of 'developer anxiety' I get is when I send arguments to an instance method that mutate both the instance and the argument. For example, Passenger.book() mutates both the Passenger and Flight instances at the same time. Is this bad design?

In case you are wondering, this is not homework.

#!/usr/bin/env python3
import random

letters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'


class BookingClass:
    ECONOMY = 'Economy'
    BUSINESS = 'Business'


class Seat:
    def __init__(self, row_number: int, letter: str):
        self.row_number = row_number
        self.letter = letter

    def __str__(self):
        return '{}{}'.format(self.row_number, self.letter)


class SeatingArea:
    def __init__(self, booking_class: str, start_row: int, row_count: int, seats_per_row: int):
        self.booking_class = booking_class
        self.seat_count = row_count * seats_per_row
        self.seats_remaining = []

        for row_number in range(start_row, row_count + start_row):
            for seat_number in range(seats_per_row):
                new_seat = Seat(row_number, letters[seat_number])
                self.seats_remaining.append(new_seat)


class Flight:
    def __init__(self, economy_seats: SeatingArea, business_seats: SeatingArea):
        # should I use the SeatingArea.booking_class attribute as the key?
        self.seating_areas = {
            BookingClass.ECONOMY: economy_seats,
            BookingClass.BUSINESS: business_seats}

    def print_statistics(self):
        for booking_class, seating_area in self.seating_areas.items():
            print('{}: {}% available'.format(
                booking_class,
                len(seating_area.seats_remaining) / seating_area.seat_count * 100))

    def remaining_seat_count(self, booking_class: str):
        return len(self.seating_areas[booking_class].seats_remaining)

    def get_seat(self, booking_class: str):
        return self.seating_areas[booking_class].seats_remaining.pop()


class Passenger:
    def __init__(self, name: str):
        self.name = name
        self.booking_id = None
        self.seat = None

    def has_booked(self):
        return self.booking_id is not None

    def book(self, flight: Flight, booking_class: str):
        if flight.remaining_seat_count(booking_class) != 0:
            self.seat = flight.get_seat(booking_class)
            self.booking_id = random.Random().randrange(1000, 10000)
            return True

        return False


# Example usage
def main():
    business = SeatingArea(BookingClass.BUSINESS, start_row=1, row_count=5, seats_per_row=4)
    economy = SeatingArea(BookingClass.ECONOMY, start_row=6, row_count=20, seats_per_row=6)

    flight = Flight(economy, business)

    john = Passenger('John')
    john.book(flight, BookingClass.ECONOMY)
    print(john.seat)
    print(john.booking_id)

    flight.print_statistics()


if __name__ == '__main__':
    main()
\$\endgroup\$
2
\$\begingroup\$

Your hardcoded letters is easier to take from string (just in case the alphabet ever changes or to save you some typing in some future code you might write):

from string import ascii_uppercase as letters

I don't see the advantage of

class BookingClass:
    ECONOMY = 'Economy'
    BUSINESS = 'Business'
...
    self.seating_areas = {
            BookingClass.ECONOMY: economy_seats,
            BookingClass.BUSINESS: business_seats}

over

self.seating_areas = {
        "economy": economy_seats,
        "business: business_seats}

The __str__ of Seat could be slightly simplified:

def __str__(self):
    return '{0.row_number}{0.letter}'.format(self)

In the constructor of SeatingArea you don't need the intermediate variable:

    for row_number in range(start_row, row_count + start_row):
        for seat_number in range(seats_per_row):
            self.seats_remaining.append(Seat(row_number, letters[seat_number]))

Assigning the booking ID like this:

self.booking_id = random.Random().randrange(1000, 10000)

will mean a roughly a 1:9000 chance of a collision. I don't know how many customers this is supposed to deal with, but that does not seem too unrealistic for e.g. a year of running. Maybe give the Flight a name and date/time like this and use that together with the seat and hash it with md5 to get a more unique id:

from datetime import datetime
from hashlib import md5
...

class Flight:
    def __init__(self, 
                 path: str,
                 date: datetime,
                 economy_seats: SeatingArea,
                 business_seats: SeatingArea):
        ...
        self.path = path
        self.date = date

...

class Passenger:
    ...
    def book(self, flight: Flight, booking_class: str):
        ...
        id_str = "{0.path}{0.date}{1.seat}".format(flight, self)
        self.booking_id = md5(id_str.encode()).hexdigest()

...

    flight_time = datetime.strptime("2016-08-12 8:00:00 -0800" "%Y-%m-%d %H:%M:%S %z")
    flight = Flight("LAX->JFK", flight_time, economy, business)

This way every seat on every flight between two airports for every different starting time has a different id (at least the chance of collisions for md5 are a lot less than 1:9000).

In reality you would probably also have a Plane class, which has the SeatingArea's as parameters and just pass that on to Flight as the plane servicing that particular flight.

\$\endgroup\$
  • \$\begingroup\$ Appreciate the answer. If I may say one thing, the review is focused more on the 'flight booking system' part than the 'oop design' part . Could you elaborate on that please? \$\endgroup\$ – AwesomeSauce Aug 15 '16 at 12:59
  • \$\begingroup\$ As mentioned in help center any part of your code is fair game. And I'm just not knowledgeable enough in OOP design to pass judgement on that part of your code. That being said, your design seemed natural enough to me (with the inclusion of flight path, start time to Flight and maybe a Plane class instead of passing the SeatingAreas directly to the flight). \$\endgroup\$ – Graipher Aug 15 '16 at 14:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.