HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Parking lot and vehicle classes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lotclassesandvehicleparking

Problem

The expected functionality is that a user/vehicle interacts with a 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
end


Please 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. @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_accessor

You'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.