patternrubyMinor
Parking lot and vehicle classes
Viewed 0 times
lotclassesandvehicleparking
Problem
The expected functionality is that a user/vehicle interacts with a
Please mention the principle in use too.
ParkingLot and it allots the space in Park method based on type of vehicle (here I can expect a few changes, like increase from regular and handicapped to one more functionality for celebrities etc). What would be the route to perfect object oriented code?class Vehicle
attr_accessor :regno,:type
def initialize(regno,type)
@regno=regno
@type=type
end
end
class ParkingLot
attr_accessor :size , :handicapped_size , :regular_size
def initialize(size)
@lot={}
@size=size
@handicapped_size=(0.1)*size
@regular_size=size-@handicapped_size
end
def check_regular
@regular_size>0 ? @regular_size-=1 : false
end
def check_handicapped
@handicapped_size>0 ? @handicapped_size-=1 : false
end
def park(vehicle,hour)
case vehicle.type
when "regular"
check_regular ? @lot[vehicle.regno]=Regular_ParkingSpace.new(hour).payment : error()
when "handicapped"
check_handicapped ? @lot[vehicle.regno]=Handicapped_ParkingSpace.new(hour).payment : error()
end
end
def unpark(vehicle)
pay=@lot[vehicle.regno]
puts "Pls pay us #{pay} rupees"
@lot.delete(vehicle.regno){|el| puts "#{el} not found in this parking lot \n"}
end
def error
raise "No more vehicles can be parked !! \n"
end
end
class ParkingSpace
attr_accessor :hour,:rate
def initialize(hour,rate)
@hour=hour
@rate=rate
end
def payment
@hour * @rate
end
end
class Regular_ParkingSpace < ParkingSpace
def initialize(hour,rate=20)
super
end
def payment
super
end
end
class Handicapped_ParkingSpace < ParkingSpace
def initialize(hour,rate=5)
super
end
endPlease mention the principle in use too.
Solution
-
Indentation and whitespace
The Ruby convention is 2 spaces of indentation, and blank lines between methods. I'd also recommend spaces between arguments and operators, e.g.
-
Naming
Don't use underscores in class names. It's clear that
-
You're adding a number of synthesized accessor methods with
-
Junk code
It sounds harsh, but I just mean "code that doesn't actually do anything". For example the
-
Dangerous assumptions
Speaking of the parking lot's size, your way of determining the number of handicap spaces is not vary robust. You just assume it's going to be one tenth of the spaces. Well, what if the parking lot has 8 spaces in total? Then you have 0.8 of a handicap space, and 7.2 regular ones. Or what if it has 213 spaces? Then you have 21.3 handicap spaces. Neither situation makes any sense. For that matter who says there are any handicap spaces at all? There's no reason - that I know if - to assume there's any proportional relationship between the two numbers.
-
Outright bugs
Following from the above: The way you check for remaining spaces assumes integers. If the remaining number of spaces is anything above zero, you assume that that means there's a whole parking space there. So, in turn, you assume that whatever
End result is that if I make a parking lot with 8 spaces, I can fit 9 vehicles: 1 handicap vehicle (in the 0.8 of a space), and 8 regular ones (the first 7 get a space each, and the last one has to fit in 0.2 of a space).
Oh, and I can just park a car for zero hours, and I'll pay zero rupees.
-
Pointless classes
You parking lot classes don't really serve any purpose being classes. You instantiate one of them, only to call
-
Informal exceptions
Don't just
In other words, there's a lot going on here. Making it "more object-oriented" is a secondary concern. And with no concept of time passing, hourly rates don't really make much difference. It's a little weird that the cost gets calculated immediately when parking, and "paid" when the car's retrieved. A real parking lot would do one or the other: Pre-pay for x amount of time (with the possibility of an extra fee if you overstay), or pay for time used when leaving. This is neither of those.
Indentation and whitespace
The Ruby convention is 2 spaces of indentation, and blank lines between methods. I'd also recommend spaces between arguments and operators, e.g.
@hour = hour.-
Naming
Don't use underscores in class names. It's clear that
Regular_ParkingSpace is a kind of ParkingSpace because it inherit directly from that parent class. A name like RegularSpace would be more straightforward and less of a mouthful.-
attr_accessorYou're adding a number of synthesized accessor methods with
attr_accessor but you never use those methods. Instead you access instance variables directly. My advice is to always use accessor methods when you can. However, attr_accessor generates both readers and writers, and they're public. Which means that external code can just say parking_lot.size = 9999, which doesn't make sense. Of course, you never actually use the size attribute for anything, which brings me to my next point:-
Junk code
It sounds harsh, but I just mean "code that doesn't actually do anything". For example the
@size variable in ParkingLot which is never used. Or Regular_ParkingSpace having a payment method that just calls super - something that'd happen automatically if the method wasn't there.-
Dangerous assumptions
Speaking of the parking lot's size, your way of determining the number of handicap spaces is not vary robust. You just assume it's going to be one tenth of the spaces. Well, what if the parking lot has 8 spaces in total? Then you have 0.8 of a handicap space, and 7.2 regular ones. Or what if it has 213 spaces? Then you have 21.3 handicap spaces. Neither situation makes any sense. For that matter who says there are any handicap spaces at all? There's no reason - that I know if - to assume there's any proportional relationship between the two numbers.
-
Outright bugs
Following from the above: The way you check for remaining spaces assumes integers. If the remaining number of spaces is anything above zero, you assume that that means there's a whole parking space there. So, in turn, you assume that whatever
size was originally passed to ParkingLot.new is cleanly divisible by 10. But that's not given.End result is that if I make a parking lot with 8 spaces, I can fit 9 vehicles: 1 handicap vehicle (in the 0.8 of a space), and 8 regular ones (the first 7 get a space each, and the last one has to fit in 0.2 of a space).
Oh, and I can just park a car for zero hours, and I'll pay zero rupees.
-
Pointless classes
You parking lot classes don't really serve any purpose being classes. You instantiate one of them, only to call
payment and then discard the instance. In the end, you classes could be replaced with methods, or even just an expression: a * b. -
Informal exceptions
Don't just
raise a string; create an exception class that inherits from StandardError and raise it instead.In other words, there's a lot going on here. Making it "more object-oriented" is a secondary concern. And with no concept of time passing, hourly rates don't really make much difference. It's a little weird that the cost gets calculated immediately when parking, and "paid" when the car's retrieved. A real parking lot would do one or the other: Pre-pay for x amount of time (with the possibility of an extra fee if you overstay), or pay for time used when leaving. This is neither of those.
Context
StackExchange Code Review Q#94270, answer score: 5
Revisions (0)
No revisions yet.