4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ ...Are you asking which design would be best to use, or do you want a review of your current code? \$\endgroup\$ – Nic Hartley Jun 21 '15 at 23:04
  • \$\begingroup\$ Both if there are any design improvements you can mention that that would be much helpful. \$\endgroup\$ – Anony-mouse Jun 22 '15 at 6:49
  • \$\begingroup\$ It took me a moment to realize that regno is not actually italian. \$\endgroup\$ – Raimund Krämer Feb 12 at 11:25
1
\$\begingroup\$

Since @Flambino critiqued your existing code, I will focus more on application architecture.

When I approach this subject, first I determine what "things" are interacting in the system.

  • Parking lots
  • Vehicles
  • Parking fees (of which there are "unrestricted" and "handicapped")
  • Payments
  • Parking spaces (of which there are "unrestricted" and "handicapped")

This gives us 5 classes. Classes are nouns (people, places or things) and methods are the verbs (actions). We will throw in two more classes to descibe the permits a vehicle can have (a handicapped sticker for those in the USA) and a custom error class for more intelligent error handling in the application.

  1. Vehicle
  2. ParkingFee
  3. ParkingSpace
  4. ParkingSpacePayment
  5. ParkingLot
  6. VehiclePermit
  7. ParkingSpaceError

We'll get the easiest one of of the way:

class ParkingSpaceError < StandardError
end

Now we've got a specific error class when problems arise parking or paying for a parking spot allowing more intelligent error handling.

Once I've identified the basic "things" in the system, I think of how they are related. The first class I build is the one with no relations to anything. It's quick and simple: The ParkingFee class.

The ParkingFee class

A parking fee consists of a rate multiplied by how many hours you've parked. Plus, you only have two rates. We will create static properties for both fee types that we can reference later:

class ParkingFee
  def self.unrestricted
    @@unrestricted ||= ParkingFee.new 20
  end

  def self.handicapped
    @@handicapped ||= ParkingFee.new 5
  end

  def initialize(rate)
    @rate = rate;
  end

  def calculate(hours)
    rate * hours
  end

  def rate
    @rate
  end
end

Easy peasy, rice and cheesey. The next class that's easy to create is the VehiclePermit class.

The VehiclePermit class

Since you must determine whether or not a vehicle is a "handicapped vehicle," this got me thinking. First, I'm assuming since you reference "rupees" in your post that you are in India, or are building software that will be used in India. In the USA, vehicles can have multiple additional attributes describing how they can be used, and where they can be parked. I believe the term that we use is "permit."

Without understanding how vehicles are registered as "handicap" vehicles in India, I'll call this next class VehiclePermit. As we will see later, every Vehicle could have multiple permits attached to it. A "handicapped" vehicle is just a Vehicle that has the handicapped VehiclePermit associated with it.

This class doesn't have much to it. An id and a name, plus a static property for convenience. This could be just as easily mapped from a database if you want.

class VehiclePermit
  def self.handicapped
    @@handicapped ||= VehiclePermit.new 1, "Handicapped"
  end

  def initialize(id, name)
    @id = id;
    @name = name;
  end

  def id
    @id
  end

  def name
    @name
  end
end

Now we can move on to the "thing" this is all about: vehicles.

The Vehicle class

This class needs one or more permits, and a registration number. Still, not very complex. We'll also expose a public method to test this vehicle to see if it has the "handicapped" permit, which we will use later when parking the vehicle.

class Vehicle
  def initialize(registration_number)
    @registration_number = registration_number
    @permits = []
  end

  def is_handicapped?
    @permits.any? {|p| p == VehiclePermit.handicapped }
  end

  def permits
    @permits
  end

  def registration_number
    @registration_number
  end
end

We have our ParkingFee, VehiclePermit and Vehicle, now we are ready to park this thing.

The ParkingSpace class

The ParkingSpace class holds a vehicle, the date and time the vehicle parked, and the fee associated with it. It allows you to calculate the payment as well. We'll define public methods for all the actions you can perform on a parking space:

  • Park a car
  • Vacate the space
  • Calculate the fee
  • Test to see if a vehicle is currently occupying it
  • Test to see if it contains a particular vehicle
  • Test a vehicle to see if it can park here

The code:

class ParkingSpace
  def initialize(parking_lot, fee, number)
    @parking_lot = parking_lot
    @fee = fee
    @number = number
  end

  def payment
    ParkingSpacePayment.new self
  end

  def can_park?(vehicle)
    !occupied?
  end

  def contains?(vehicle)
    self.vehicle == vehicle
  end

  def number
    @number
  end

  def occupied?
    !vehicle.nil?
  end

  def park(vehicle)
    raise ParkingSpaceError "Cannot park vehicle #{vehicle.registration_number}" unless can_park? vehicle
    self.vehicle = vehicle
    date_occupied = DateTime.current
  end

  def vacate
    payment = nil
    date_occupied = nil
    vehicle = nil
  end

  def vehicle
    @vehicle
  end

private

  def payment=(new_payment)
    @payment = new_payment
  end

  def calculate_fee(date)
    fee.calculate(date)
  end

  def vehicle=(new_vehicle)
    @vehicle = new_vehicle
  end
end

We aren't done yet, because there are two kinds of parking spaces. We will define concrete classes for each:

class UnrestrictedParkingSpace < ParkingSpace
  def initialize(parking_lot, number)
    super(parking_lot, ParkingFee.unrestricted, number)
  end
end

An "unrestricted" parking space doesn't require much code. We just override the constructor to pass the proper ParkingFee object. The can_park? method on the parent class has the correct logic for this type of space, so we don't bother overriding it.

class HandicappedParkingSpace < ParkingSpace
  def initialize(parking_lot, number)
    super(parking_lot, ParkingFee.handicapped, number)
  end

  def can_park?(vehicle)
    super(vehicle) && vehicle.is_handicapped?
  end
end

The "handicapped" parking space overrides the constructor, which passes the handicapped fee, and overrides the can_park? method. The can_park? method first delegates to the method on the super class, and if that returns true, we have an additional test to see if the vehicle has a handicapped permit, so we also call Vehicle#can_park?.

Each type of parking space has the fee baked in so no one can make the "Unrestricted" parking space fee less that 20 Rupees. Now that we've got a parking spaces, we need some place to put them: the ParkingLot.

The ParkingLot class

A parking lot has a bunch of spaces, both unrestricted and handicapped. We also need to do the following things in the parking lot:

  • Park a vehicle
  • Exit the parking lot
  • Calculate a payment for a vehicle
  • Check to see if spaces are available

The constructor for the ParkingLot class takes two arguments: the number of unrestricted spaces and the number of handicapped spaces.

class ParkingLot
  def initialize(unrestricted_count, handicapped_count)
    count = 0
    @parking_spaces = []

    unrestricted_count.times do |n|
      @parking_spaces << UnrestrictedParkingSpace.new self, n
    end

    count = @parking_spaces.count

    handicapped_count.times do |n|
      @parking_spaces << HandicappedParkingSpace.new self, count + n
    end
  end

  def exit(vehicle, payment)
    parking_space = parking_space_for vehicle
    raise ParkingSpaceError "Balance not paid: #{payment.balance}" unless payment.paid?
    parking_space.vacate vehicle
    parking_space
  end

  def park(vehicle)
    index = @parking_spaces.index {|space| space.can_park? vehicle }
    raise ParkingSpaceError "No spaces available" if index < 0
    parking_space = @parking_spaces[index]
    parking_space.park vehicle
    parking_space
  end

  def payment_for(vehicle)
    parking_space_for(vehicle).payment
  end

  def spaces_available?(vehicle = nil)
    if vehicle.nil?
      @parking_spaces.any? {|space| !space.occupied? }
    else
      @parking_spaces.any? {|space| space.can_park? vehicle }
    end
  end

private

  def parking_space_for(vehicle)
    index = @parking_spaces.index {|space| space.contains? vehicle }
    raise ParkingSpaceError "Vehicle #{vehicle.registration_number} is not parked in this lot" if index < 0
    @parking_spaces[index]
  end
end

Lastly, let's pay for our spot using a ParkingSpacePayment

The ParkingSpacePayment class

This class contains all the logic for paying for a parking spot. It needs the parking space, vehicle and fee. All three things are provided by the ParkingSpace object, so we require this in the constructor.

class ParkingSpacePayment
  def initialize(parking_space)
    @parking_space = parking_space
    @payment_date = DateTime.current
    @total_hours = ((@payment_date - parking_space.date_occupied) / 1.hour).round
    @amount_due = @parking_space.calculate_fee @total_hours
    @amount_paid = 0
  end

  def amount_due
    @amount_due
  end

  def amount_paid
    @amount_paid
  end

  def balance
    @amount_due - @amount_paid
  end

  def paid?
    @amount_paid <= 0
  end

  def parking_space
    @parking_space
  end

  def pay(amount)
    @amount_paid += amount
    balance
  end

  def payment_date
    @payment_date
  end

  def total_hours
    @total_hours
  end
end

Using these classes to manage a parking lot

Now that we've got all the pieces built, let's see how we can park a car, and pay for it.

begin
  # Our parking lot as 10 unrestricted spaces and 5 handicapped spaces
  parking_lot = ParkingLot.new 10, 5

  # Create two vehicles, one of each type.
  unrestricted_vehicle = Vehicle.new "1"
  handicapped_vehicle = Vehicle.new "2"
  handicapped_vehicle.permits << VehiclePermit.handicapped

  # Park the cars
  parking_lot.park unrestricted_vehicle
  parking_lot.park handicapped_vehicle

  # Let's do some shopping.
  sleep 3.hours

  # Time to check out.
  unrestricted_payment = parking_lot.payment_for unrestricted_vehicle
  handicapped_payment parking_lot.payment_for handicapped_vehicle

  # 20 * 3 hours should be 60. Opps! This could be a problem later
  unrestricted_payment.pay 55

  # 5 * 3 hours = 15
  handicapped_payment.pay 15

  # I guess we went to the ATM. Whew!
  unrestricted_payment.pay 5 unless unrestricted_payment.paid?

  # Now let's exit the parking lot. Time to go home.
  unrestricted_space = parking_lot.exit unrestricted_vehicle, unrestricted_payment
  puts unrestricted_space.occupied? # -> "false"

  handicapped_space = parking_lot.exit handicapped_vehicle, handicapped_payment
  puts handicapped_space.occupied? # -> "false"
rescue ParkingSpaceError => parking_error
  puts "Oops! Had a problem parking a car: #{parking_error}"
else Exception => e
  raise e # A non parking error occurred.
end

Extra Credit

Since a parking lot could have multiple levels and rows on each level, the layout of a parking lot could be abstracted away to a ParkingLotLayout class, which takes a ParkingLot as a constructor argument and defines the floors and rows that are available.

\$\endgroup\$
  • \$\begingroup\$ absolutely wonderful answer. Where can I gain this much insight into the designs ? Please help. \$\endgroup\$ – Anony-mouse Jul 24 '15 at 16:01
  • \$\begingroup\$ this was done for fun, just to learn design principles(I'm just out of college). It's not going into production. Even though with your code it can be. \$\endgroup\$ – Anony-mouse Jul 24 '15 at 16:06
  • \$\begingroup\$ @Anony-mouse: Start out with SOLID, which is a set of foundational principals for object oriented development. Also search for "software design patterns" to learn which patterns are out there. Lastly, the "Fat Model, Skinny Controller" methodology that Ruby on Rails enthusiasts have bolstered for years has a more formal definition with Domain Driven Design. \$\endgroup\$ – Greg Burghardt Jul 24 '15 at 16:20
  • \$\begingroup\$ I am done with SOLID (Uncle bob martin and c2.com). Even though I have still issues in understanding the direct application of the Principles in Ruby. Currently I am after TDD, and it has substantially increased my ability to write good code.Can you like give me your email id or something as the discussions are personal and I have more things to ask(that is if you do not mind) \$\endgroup\$ – Anony-mouse Jul 24 '15 at 16:26
5
\$\begingroup\$
  1. 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.

  2. 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.

  3. 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:

  4. 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.

  5. 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.

  6. 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.

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

  8. 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.

\$\endgroup\$
  • \$\begingroup\$ wow that was nice any other improvements pertaining to the design ie like it violates SRP etc \$\endgroup\$ – Anony-mouse Jun 22 '15 at 6:51
  • \$\begingroup\$ "Don't just raise a string; create an exception class that inherits from StandardError and raise it instead." Er, why? I'm sure there's a perfectly valid reason, but could you explain that? \$\endgroup\$ – Nic Hartley Jun 22 '15 at 11:07
  • \$\begingroup\$ @QPaysTaxes Rescuing an arbitrary string is messier than rescuing from a named error type. Besides, there might be other, different exceptions you'd want to raise - like if try to park a car with no reg. number or something. Such things could have their own exception classes, making them easier to handle. The docs have a guideline that explains it well (4th paragraph of the intro). Obviously it's not required for an exercise, but best practices and all that. \$\endgroup\$ – Flambino Jun 22 '15 at 12:45
  • \$\begingroup\$ @Flambino Oh, I see. Thanks! (Crap, time to rewrite some code...) \$\endgroup\$ – Nic Hartley Jun 22 '15 at 15:01
  • \$\begingroup\$ Thank you for the help if there is any design issues please tell me.I am marking it correct.Can you propose some good books to learn design ? \$\endgroup\$ – Anony-mouse Jun 24 '15 at 13:14

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.