7
\$\begingroup\$

Follow up for my initial question. Modified my earlier code and adding new changes. Initial Question in Code Review

import random

class Booking:
    def __init__(self):
        pass

    def checkAvailability(self,**kwargs):
        booking_type = kwargs['booking_type']
        booking_class = kwargs['booking_class']
        if booking_type == "Airline":
            departure_date = kwargs['departure_date']
            airline = kwargs['airline']
            if airline.AIRLINE_SEATS[booking_class] != 0 :
                print "Flight Available\n"
                return True
            else:
                print "Flight Not Available\n"
                return False
        elif booking_type == "Hotel":
            if H1.HOTEL_ROOM[booking_class] != 0:
                print "Hotel Available\n"
                return True
            else:
                print "Hotel Not Available\n"
                return False

    def makeReservation(self,**kwargs):
        booking_type = kwargs['booking_type']
        customer = kwargs['customer']
        availability_status = kwargs['availability_status']
        if (availability_status == True) and (booking_type == "Airline"):
            booking_class = kwargs['booking_class']
            airline = kwargs['airline']
            airline.AIRLINE_SEATS[booking_class] -= 1
            customer.customer_record['Wallet'] += airline.AIRLINE_PRICE[booking_class]
            customer.customer_record['Booking ID']['Airline'] = "AIR" + str(random.randrange(10,1000,2))
        elif (availability_status == True) and (booking_type == "Hotel"):
            booking_class = kwargs['booking_class']
            hotel = kwargs['hotel']
            hotel.HOTEL_ROOM[booking_class] -= 1
            customer.customer_record['Wallet'] += hotel.HOTEL_PRICE[booking_class]
            customer.customer_record['Booking ID']['Hotel'] = "HOT" + str(random.randrange(10,1000,2))


class Airline(Booking):
    def __init__(self,airline_name):
        Booking.__init__(self)
        self.airline_name = airline_name
        self.AIRLINE_SEATS = { 'Business Class' : 50,
                               'First Class'    : 50,
                               'Premium Economy': 100,
                               'Regular Economy': 150
                             }
        self.AIRLINE_PRICE = { 'Business Class' : 2500,
                               'First Class'    : 2000,
                               'Premium Economy': 1800,
                               'Regular Economy': 1500
                             }

class Hotel(Booking):
    def __init__(self,hotel_name):
        Booking.__init__(self)
        self.hotel_name = hotel_name
        self.HOTEL_ROOM = {'Penthouse'             : 10,
                           'King Deluxe Bedroom'    : 20,
                           'Queen Deluxe Bedroom'   : 20,
                           'Kind Standard Bedroom' : 30,
                           'Queen Standard Bedroom': 50
                           }
        self.HOTEL_PRICE = {'Penthouse'            : 1000,
                            'King Deluxe Bedroom'    : 700,
                            'Queen Deluxe Bedroom'   : 600,
                            'Kind Standard Bedroom' : 450,
                            'Queen Standard Bedroom': 350
                           }

class Customer:
    def __init__(self,f_name,l_name):
        self.f_name = f_name
        self.l_name = l_name
        self.cost = 0
        self.customer_record = {'Name'  : self.f_name + " " + self.l_name,
                                'Wallet': self.cost,
                                'Booking ID' : {}
                               }
    def printcustomerrecord(self):
            for k,v in self.customer_record.items():
                print k + "\t" + str(v) + "\n"


C1 = Customer("Wayne","Rooney")

A1 = Airline("Qantas")
availability = A1.checkAvailability(airline = A1, booking_type = "Airline", booking_class = "Business Class", departure_date = "07072014")
A1.makeReservation(airline = A1, customer = C1, booking_type = "Airline", booking_class = "Business Class", availability_status = availability)
H1 = Hotel("Sheraton")
availability = H1.checkAvailability(airline = H1, booking_type = "Hotel", booking_class = "Penthouse", checkin = "07072014", checkout = "07102014")
H1.makeReservation(hotel = H1, customer = C1, booking_type = "Hotel", booking_class = "Penthouse", availability_status = availability)
C1.printcustomerrecord()

I would really want some clarification on calling the

makeReservation 
checkAvailability 

calls from their objects where i pass the objects as a parameter. I don't know if thats a proper way to do it. If there is a better way to do. I would be really happy to know about it.

\$\endgroup\$
  • 3
    \$\begingroup\$ I have got the feeling that you did not really take into account all of the advice from the previous review (e.g. using class attributes instead of instance attributes) :/ \$\endgroup\$ – Morwenn May 7 '14 at 11:51
13
\$\begingroup\$

Polymorphism

The biggest problem I see is that you're not using polymorphism at all. Instead of calling methods on Booking and using if-else to decide if it's a hotel or airline you're trying to book, you should implement the specialized logic in Hotel and Airline. For example like this:

class Bookable(object):
    def is_available(self, **kwargs):
        raise NotImplementedError

    def make_reservation(self, **kwargs):
        raise NotImplementedError


class Airline(Bookable):    
    def is_available(self, booking_class):
        return self.seats[booking_class] > 0

    def make_reservation(self, customer, booking_class):
        if self.is_available(booking_class):
            self.seats[booking_class] -= 1
            customer.customer_record['Wallet'] += self.prices[booking_class]

Python standards

This is old-style class declaration:

class Customer:
    pass

Use this new style instead:

class Customer(object):
    pass

The modern way of calling superclass methods:

class Airline(Bookable):
    def __init__(self, airline_name):
        super(Airline, self).__init__()

If a method is unused, delete from your code, or at least omit from code review:

def __init__(self):
    pass

This has many unnecessary elements:

if (availability_status == True) and (booking_type == "Airline"):

You can simplify to:

if availability_status and booking_type == "Airline":

Naming

There are many many naming issues in your code:

  • Objects should have meaningful, lowercase names. For example instead of C1, call it customer, instead of A1, call it airline1
  • Don't name instance attributes with all caps, for example AIRLINE_SEATS could have been simply seats

An example

I would simplify Airline like this:

class Airline(Bookable):
    CLASS_BUSINESS = 'Business Class'
    CLASS_FIRST = 'First Class'
    CLASS_PREMIUM_ECONOMY = 'Premium Economy'
    CLASS_ECONOMY = 'Regular Economy'

    def __init__(self, airline_name):
        super(Airline, self).__init__()
        self.airline_name = airline_name
        self.seats = {
            self.CLASS_BUSINESS: 50,
            self.CLASS_FIRST: 50,
            self.CLASS_PREMIUM_ECONOMY: 100,
            self.CLASS_ECONOMY: 150,
        }
        self.prices = {
            self.CLASS_BUSINESS: 2500,
            self.CLASS_FIRST: 2000,
            self.CLASS_PREMIUM_ECONOMY: 1800,
            self.CLASS_ECONOMY: 1500,
        }

    def is_available(self, booking_class):
        return self.seats[booking_class] > 0

    def make_reservation(self, booking_class):
        if self.is_available(booking_class):
            self.seats[booking_class] -= 1

Your testing code example could be written as:

airline = Airline("Quantas")
booking_class = Airline.CLASS_BUSINESS
airline.make_reservation(booking_class)

I removed Customer from this example on purpose. An Airline should not know about the implementation details of a Customer, like in your post, updating customer.customer_record['Wallet']. At the minimum, Customer should have API methods like add_cost, hiding the implementation details. To go even further, the Airline should not update a Customer, it would be better to move that logic to a new class, something like a BookingManager.

UPDATE: another example

You're right that Bookable is not doing much useful in the previous simplified example. Here's another implementation example, with common logic moved from Airline to Bookable in a way that Hotel can reuse it too:

class Bookable(object):
    def is_available(self, booking_class):
        return self.get_items(booking_class) > 0

    def make_reservation(self, customer, booking_class):
        if self.is_available(booking_class):
            self.decrement_items(booking_class)
            customer.customer_record['Wallet'] += self.get_price(booking_class)
            customer.customer_record['Booking ID'][self.get_class_name()] = self.get_id_prefix() + str(random.randrange(10, 1000, 2))

    def get_class_name(self):
        return self.__class__.__name__

    def get_items(self, booking_class):
        raise NotImplementedError

    def get_price(self, booking_class):
        raise NotImplementedError

    def decrement_items(self, booking_class):
        raise NotImplementedError

    def get_id_prefix(self):
        raise NotImplementedError


class Airline(Bookable):
    CLASS_BUSINESS = 'Business Class'
    CLASS_FIRST = 'First Class'
    CLASS_PREMIUM_ECONOMY = 'Premium Economy'
    CLASS_ECONOMY = 'Regular Economy'

    def __init__(self, airline_name):
        super(Airline, self).__init__()
        self.airline_name = airline_name
        self.seats = {
            self.CLASS_BUSINESS: 50,
            self.CLASS_FIRST: 50,
            self.CLASS_PREMIUM_ECONOMY: 100,
            self.CLASS_ECONOMY: 150,
        }
        self.prices = {
            self.CLASS_BUSINESS: 2500,
            self.CLASS_FIRST: 2000,
            self.CLASS_PREMIUM_ECONOMY: 1800,
            self.CLASS_ECONOMY: 1500,
        }

    def get_items(self, booking_class):
        return self.seats[booking_class]

    def decrement_items(self, booking_class):
        self.seats[booking_class] -= 1

    def get_id_prefix(self):
        return 'AIR'

    def get_price(self, booking_class):
        return self.prices[booking_class]
\$\endgroup\$
  • \$\begingroup\$ if i am going to raise NotImplementedError in each of the methods in Booking class, I might as well not use that Booking class at all. I can just have two Airline and Hotel class ?. I am newbie to OOPs, so i am trying to clarify this here. How should I abstract these two methods as one in parent. They are both doing a similar functionality, but they are just acting on different data. \$\endgroup\$ – Arman May 9 '14 at 2:54
  • \$\begingroup\$ @Arman fair enough ;-) I added another, more detailed example where Bookable has some methods that both Airline and Hotel can benefit from. \$\endgroup\$ – janos May 9 '14 at 5:57
4
\$\begingroup\$
def __init__(self):
    pass

The init call can be deleted.

def checkAvailability(self,**kwargs):
    booking_type = kwargs['booking_type']
    booking_class = kwargs['booking_class']

Be careful with **kwargs -- it's flexible but provides no clue as to what the function/method is actually expecting. You may want to add a doc string, or explicitly state which keywords the function expects in its signature. Regardless, you may want to consider adding either a check or a default if the keyword isn't there:

if 'booking_type' in kwargs:
    booking_type = kwargs['booking_type']

or

booking_type = kwargs.get('booking_type', default_booking_value)

As Morwenn also suggests, you may want to go back and look at the info in your previous answer -- right now you've still got logic for both airlines and hotels in your Booking class. In something like your check_availability method, that logic can be abstracted to see if a generalize resource is available, rather than being specifically for Hotels/Airlines/Jabberwockies/FooBars/Etc..

\$\endgroup\$

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