5
\$\begingroup\$

Please review my code for parking lot design. I am new to OOD concepts, It will be great to have some feedback on OO Structure of the solution.

Requirements considered:

  1. The parking lot can have multiple levels.
  2. Each level can have 'compact', 'large', 'bike' and 'electric' spots.
  3. Vehicle should be charged according to spot type, time of parking and duration of parking.
  4. Should be able to add more spots to level.
  5. Show the available number of spots on each level.
class parkingFloor():
    def __init__(self,name):
        self.name = name
        self.spotTotal = {'compact':0,'large':0,'bike':0,'electric':0}

        self.spotTaken = {'compact':0,'large':0,'bike':0,'electric':0}

        self.freeSpot = {'compact':set(),'large':set(),'bike':set(),'electric':set()}
        self.takenSpot = {'compact':{},'large':{},'bike':{},'electric':{}}

    def assignSpot(self,tickt):
        if self.spotTaken[tickt.veh.type] >= self.spotTotal[tickt.veh.type]:
            return False
        for s in self.freeSpot[tickt.veh.type]:
            if s.id not in self.takenSpot[tickt.veh.type]:
                self.takenSpot[tickt.veh.type][s.id] = tickt
            self.spotTaken[tickt.veh.type]+=1
            self.freeSpot[tickt.veh.type].remove(s)
            tickt.allocateSpot(s)
            return True
        return False

    def addSpot(self,type,v):
        for i in range(v):
            s = spot(type)
            self.freeSpot[type].add(s)
        self.spotTotal[type] += v

class entryPanel():
    def __init__(self,id):
        self.id = id

    def printTicket(self,tickt):
        print('Vehicle ID ',tickt.veh.id)
        print('Spot ID ',tickt.spot.id)
        print('Ticket ID ',tickt.id)
        print('Date Time',tickt.DateTime)

    def display(self,message):
        print(message)


class vehicle():
    def __init__(self,id,vehType):
        self.id = id
        self.type = vehType


class spot():
    def __init__(self,spotType):
        def generateId():
            # some mechanism to generate spot id
            return 1
        self.id = generateId()
        self.type = spotType



class ticket():
    def __init__(self,v1):
        self.id = self.generateId()
        self.veh = v1

        self.spot = None
        self.DateTime = self.getTime()
        self.amount = 0
        self.status = 'Active'
        self.payment = None

    def getTime(self):
        time = 1234
        return time

    def generateId(self):
        # some mechanism to generate new ticket id
        new_ticket  = 1
        return new_ticket

    def allocateSpot(self,spot):
        self.spot = spot

    def addPayment(self,pay):
        self.status = 'Complete'
        self.payment = pay

class parkingLot():
    def __init__(self,name,address):
        self.name = name
        self.address = address
        self.level = []

    def addLevel(self,floor):
        self.level.append(floor)

    def processEntry(self,t1,gate):
        for l in self.level:
            if l.assignSpot(t1):
                gate.printTicket(t1)
                return
        gate.display('No Spot Empty')

    def processExit(self,tickt,gate):
        def getTime():
            # Gives the current time
            return 3
        currTime = getTime()
        print('Processing fare',tickt.veh.type,tickt.spot.id,tickt.DateTime,currTime)
        amountCalculated = 7
        tickt.addPayment(Payment(amountCalculated))
        gate.display('Payment Successful')

class Payment():
    def __init__(self,amount):
        self.id = 'paymentid2'
        self.type = 'credit' # debit
        self.time = 'paymet time'

class displayBoard():
    def show(self,p):
        for l in p.level:
            print(l.name)
            for k in l.spotTotal.keys():
                print(k, l.spotTotal[k] - l.spotTaken[k])


P = parkingLot('Savita','Address')
floor1 = parkingFloor('floor1')
P.addLevel(floor1)
floor1.addSpot('compact',5)

board = displayBoard()
board.show(P)

entryPanel1 = entryPanel('1')

v1 = vehicle(1,'compact')
t1 = ticket(v1)

P.processEntry(t1,entryPanel1)
P.processExit(t1,entryPanel1)

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! You've posted a lot of code to review. Consider giving the reviewers a hint where the focus of the review should be (e.g. style, clarity, OO structure, ...). Also elaborate on what the expected outcome of the provided test code should be. \$\endgroup\$ – AlexV Mar 26 at 20:53
5
\$\begingroup\$

Overall, this is actually very good, especially considering you're new to the OOP design pattern. There're just a few things I would recommend changing:

Naming conventions

There's a list of naming conventions that are widely accepted all throughout the python community. Following these conventions makes it easier to share and develop your code with other developers because everyone's already familiar with the conventions. It also makes your life easier because you have a set of guidelines to follow, and you're more likely to keep your conventions consistent, thus making your code easier to understand.

Your class names don't follow the convention of naming classes in CamelCase starting with a capital letter. So you should change parkingFloor to ParkingFloor, entryPanel to EntryPanel, etc.

Enums over strings

In several places, you're using strings where you should be using enums. Enums are basically special classes filled with constants and are preferable in certain situations when you're using the same strings in multiple places. Python does have built-in support for enums, and you should use it.

Your whole ticket class could benefit largely from this. My suggestion is to create a class called TicketStatus extending enum.Enum (thus making it an enum), and create within it the constants ACTIVE and COMPLETE. You can then change these two statements:

self.status = 'Active'

self.status = 'Complete'

to this:

self.status = TicketStatus.ACTIVE

self.status = TicketStatus.COMPLETE

Using enums makes comparisons and debugging easier, and overall makes your code a bit easier to understand.

Comments/documentation

Looking through your entire code, you have only 4 comments, 3 of which are placeholders for code you have not yet written. This is not good documentation. I can guarantee that if you stop working on your project and come back a month later, you will wish you explained to yourself what your code does and why. Or, if another developer starts working on your project, they won't know what much of the code does.

In general, you should document almost everything. This makes it easier for both yourself and others who are viewing your code. Docstrings (you may have heard them called "multiline comments") should be used when documenting a class, function, module, or method.

Inline comments should be used a little more sparingly. If anyone who has never seen your program before can easily tell what a piece of code does, you do not need to add a comment explaining it. But if the purpose of the statement is not entirely obvious, adding a comment can be useful.

\$\endgroup\$
3
\$\begingroup\$

Pikachu the Purple Wizard's answer covers quite a lot of essential style hints and best practices such as using enums.

However, he has not said a lot about the actual classes themselves. Especially displayBoard caught me attention immediately.

class displayBoard():
    def show(self,p):
        for l in p.level:
            print(l.name)
            for k in l.spotTotal.keys():
                print(k, l.spotTotal[k] - l.spotTaken[k])

This is kind of a nonsense class. Classes should generally be used to hold variables and methods that would naturally belong together. displayBoard does not hold any data, and has a single function. Unlike Java, Python allows for functions which are not part of a class. So this could easily become

def displayBoard(p):
    for l in p.level:
        print(l.name)
        for k in l.spotTotal.keys():
            print(k, l.spotTotal[k] - l.spotTaken[k])

without losing any functionality, clearity or whatever. As Pickachu said, this function would greatly benefit from documentation, and more expressive variable names (What type of object should p be?).

On the other hand, Payment is as a fully valid candidate for a class. However your code ignores the only input value one might pass to its constructor.

class Payment():
    def __init__(self, amount): #<- amount is not used anywhere
        self.id = 'paymentid2' #<- all these values may only be set afterwards, why?
        self.type = 'credit' # debit
        self.time = 'payment time'

While reworking your code, also think about whether a string is really a good fit for storing time. E.g. Python has a built-in datetime module which might be (and probably is) a better fit here.

Another thing sou should change are those nested functions sometimes found in methods or in the constructor of some classes.

class spot():
    def __init__(self,spotType):
        def generateId():
            # some mechanism to generate spot id
            return 1
        self.id = generateId()
        self.type = spotType

Should be reworked into

class Spot():
    def __init__(self, spotType):
        self.id = self.generateId()
        self.type = spotType

    def generateId(self):
        """some mechanism to generate spot id"""
        return 1

which is what you already do, e.g. for class ticket(). If you intend to show that generateId(self) is not supposed to be used outside of the class, prepend it with _. That does not really hide the function from users outside of your class, but it is generally accepted as a convention that this function is only for internal use only and might be changed/removed/... without further notice.

\$\endgroup\$
0
\$\begingroup\$

Working code for the above questions -

  1. Better naming conventions
  2. Used enums over strings

A few more changes are there since I added some more functionality like -

  1. Car / Bike classes extend the Vehicle Base Class
  2. Ticket has Vehicle, Spot and Payment objects
  3. Have added unassignSpot() as well

Other people are welcome to review my code and provide your valuable inputs. Thank you.

import time
from enum import Enum

class TicketStatus (enum.Enum) :
    ACTIVE = 1
    COMPLETE = 2
class VehicleType (enum.Enum) :
    CAR = 1
    BIKE = 2
class SpotType (enum.Enum) :
    FREE = 1
    TAKEN = 2

class ParkingLot :
    def __init__(self, name, address) :
        self.name = name
        self.address = address
        self.level = []
    def addLevel(self, floor):
        self.level.append(floor)
    def processEntry(self, ticket) :
        for eachlevel in self.level :
            if eachlevel.spots[ticket.veh.type][SpotType.FREE] :
                ticket.spot = eachlevel.assignSpot(ticket)
                print('Entry Completed For : ', ticket.veh.num)
                break;
    def processExit(self, ticket) :
        for eachlevel in self.level :
            if ticket.spot in eachlevel.spots[ticket.veh.type][SpotType.TAKEN] :
                eachlevel.unassignSpot(ticket)
                break;
        ticket.outTime  = time.time()
        ticket.spots    = None
        ticket.status   = TicketStatus.COMPLETE
        ticket.payment  = Payment(ticket.inTime, ticket.outTime)
        print('Exit Completed For  : ', ticket.veh.num, ' Pay : ', int(ticket.payment.amount))

class ParkingLevel :
    def __init__(self, name) :
        self.name = name
        self.spots = {VehicleType.CAR : {SpotType.FREE : [], SpotType.TAKEN : []}, 
                        VehicleType.BIKE : {SpotType.FREE : [], SpotType.TAKEN : []} }
    def assignSpot(self, ticket) :
        if self.spots[ticket.veh.type][SpotType.FREE] != [] :
            spot = self.spots[ticket.veh.type][SpotType.FREE].pop()
            ticket.spot = spot
            self.spots[ticket.veh.type][SpotType.TAKEN].append(spot)
            return ticket.spot
        return False
    def unassignSpot(self, ticket) :
        self.spots[ticket.veh.type][SpotType.FREE].append(ticket.spot)
        self.spots[ticket.veh.type][SpotType.TAKEN].remove(ticket.spot)
    def addSpot(self, type, num) :
        for eachnum in range(num) :
            spot = Spot(type)
            self.spots[type][SpotType.FREE].append(spot)            

class Vehicle :
    def __init__(self, num) :
        self.id     = self.generateID()
        self.num    = num
    def generateID(self) :
        yield range(100)

class Car (Vehicle) :
    def __init__(self, num) :
        super().__init__(num)
        self.type   = VehicleType.CAR

class Bike (Vehicle) :
    def __init__(self, num) :
        super().__init__(num)
        self.type   = VehicleType.CAR

class Spot :
    def __init__(self, type) :
        self.id     = self.generateID()
        self.type   = type
    def generateID(self) :
        yield range(100)

class Payment :
    def __init__(self, inTime, outTime) :
        self.mode   = None
        self.rate   = [30, 20, 10]
        self.amount = self.calAmount(inTime, outTime)
    def getRate(self) :
        return self.rate
    def setRate(self, rate) :
        self.rate = rate
    def calAmount(self, inTime, outTime) :
        amount =  (outTime - inTime) * self.getRate()[0]
        amount += (outTime - inTime - 60 ) * self.getRate()[1] if outTime - inTime - 60 > 0 else 0
        amount += (outTime - inTime - 120 ) * self.getRate()[2] if outTime - inTime - 120 > 0 else 0
        return amount        

class Ticket :
    def __init__(self, veh) :
        self.veh     = veh
        self.status  = TicketStatus.ACTIVE
        self.inTime  = time.time()
        self.outTime = None
        self.payment = None
        self.spot    = None
    def generateID(self) :
        # some ID generation mechanism
        return ID

class DisplayBoard :
    def show(self, P) :
        for eachlevel in P.level :
            print(P.name , '-' , eachlevel.name, '- Available Parking Spots')
            print('Car  : ', len(eachlevel.spots[VehicleType.CAR][SpotType.FREE]))
            print('Bike : ', len(eachlevel.spots[VehicleType.BIKE][SpotType.FREE]))


P = ParkingLot('Google Parking Lot', '123, Fort, Mumbai')
F1 = ParkingLevel('F1')
F1.addSpot(VehicleType.CAR, 3)
F1.addSpot(VehicleType.BIKE, 3)
P.addLevel(F1)

Board = DisplayBoard()
Board.show(P)

T1 = Ticket(Car('MH05 AB 5454'))
P.processEntry(T1)

T2 = Ticket(Bike('MH05 AB 9000'))
P.processEntry(T2)

time.sleep(2)  
P.processExit(T2)

Board = DisplayBoard()
Board.show(P)
\$\endgroup\$
  • 1
    \$\begingroup\$ If you want this code reviewed then you should ask a new question and include a link to the question above instead of putting it in an answer. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 26 at 15:59

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.