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\$ – Stop ongoing harm to Monica 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\$

protected by Community Nov 15 '16 at 12:31

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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