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

Simple Ruby program to model a prepaid transit card system

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

Problem

I have written a simple Ruby program to model a transit card system which aims to address the following user stories. I wanted to see whether I could get any feedback on it. My main interest is understanding where I have made fundamental mistakes that violate OOP(SOLID) Principles with a real focus on Single Responsibility.

I have included the code and specs and link to my repo - https://github.com/benhawker/oyster-cards.



-
In order to use public transport

As a customer

I want money on my card

-
In order to keep using public transport

As a customer

I want to add money to my card

-
In order to protect my money

As a customer

I don't want to put too much money on my card

-
In order to pay for my journey

As a customer

I need my fare deducted from my card

-
In order to get through the barriers

As a customer

I need to touch in and out

-
In order to pay for my journey

As a customer

I need to have the minimum amount for a single journey

-
In order to pay for my journey

As a customer

I need to pay for my journey when it's complete

-
In order to pay for my journey

As a customer

I need to know where I've travelled from

-
In order to know how far I have travelled

As a customer

I want to know what zone a station is in

-
In order to be charged correctly

As a customer

I need a penalty charge deducted if I fail to touch in or out

-
In order to be charged the correct amount

As a customer

I need to have the correct fare calculated


```
require "./lib/journey"

class Oystercard

attr_reader :balance

MIN_BALANCE = 1
MAX_BALANCE = 50

def initialize(balance=0)
raise "Max balance is #{Oystercard::MAX_BALANCE}" if exceeds_max_balance?(balance)
@balance = balance
end

def top_up(amount)
raise "This would take you over the max balance!" if exceeds_max_balance?(@balance + amount)
@balance += amount
end

def tap_in(origin_station)
raise "Yo

Solution

First, deduct(Journey::PENALTY_FARE) in class Oystercard looks weird.
There is no need to place calculation of Journey's fare in Oystercard:

class Journey

  # def calculate_fare # replaced with #fare
  #   BASE_FARE + variable_trip_price
  # end
  
  def fare
    if origin_station && destination_station
      BASE_FARE + variable_trip_price
    else
      PENALTY_FARE
    end
  end
  
  # rename #complete_journey >>> #complete_at
  # @journey.complete_at(destination_station) is more readable
  def complete_at(destination_station)
    @destination_station = destination_station
  end

private
  #...
end

class Oystercard

  def tap_out(destination_station)
    @journey ||= Journey.new(nil)
    @journey.complete_at(destination_station)
    deduct(@journey.fare)
  end

end


There is no need to call true explicitly in below_min_balance? and exceeds_max_balance?:

def below_min_balance?(balance)
  balance  MAX_BALANCE
end


Seems like you're not fully cover this part:

In order to be charged correctly

As a customer

I need a penalty charge deducted if I fail to touch in or out

When customer skips tap_out and tap_in again, previous @journey will be replaced with Journey.new, no charge applied.

If so, Oystercard::MIN_BALANCE > 5 - to prevent negative balance in that "edge-case".

Code Snippets

class Journey

  # def calculate_fare # replaced with #fare
  #   BASE_FARE + variable_trip_price
  # end
  
  def fare
    if origin_station && destination_station
      BASE_FARE + variable_trip_price
    else
      PENALTY_FARE
    end
  end
  
  # rename #complete_journey >>> #complete_at
  # @journey.complete_at(destination_station) is more readable
  def complete_at(destination_station)
    @destination_station = destination_station
  end

private
  #...
end

class Oystercard

  def tap_out(destination_station)
    @journey ||= Journey.new(nil)
    @journey.complete_at(destination_station)
    deduct(@journey.fare)
  end

end
def below_min_balance?(balance)
  balance < MIN_BALANCE
end

def exceeds_max_balance?(balance)
  balance > MAX_BALANCE
end

Context

StackExchange Code Review Q#118493, answer score: 2

Revisions (0)

No revisions yet.