Simple Ruby program to model a prepaid transit card system

Question

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 "You don't have enough for this journey :(" if below_min_balance?(@balance)
 @journey = Journey.new(origin_station)
 end
 def tap_out(destination_station)
 if @journey
 @journey.complete_journey(destination_station)
 deduct(@journey.calculate_fare)
 else
 deduct(Journey::PENALTY_FARE)
 end
 end
 private
 def deduct(amount)
 @balance -= amount
 end
 def below_min_balance?(balance)
 true if balance < MIN_BALANCE
 end
 def exceeds_max_balance?(balance)
 true if balance > MAX_BALANCE
 end
end

---

class Station
 attr_reader :zone
 MAX_ZONE = 5
 def initialize(zone = 1)
 raise "We only have 5 zones" if zone > MAX_ZONE
 @zone = zone
 end
end

---

class Journey
 attr_reader :origin_station, :destination_station
 BASE_FARE = 1
 PRICE_PER_ZONE = 1.2
 PENALTY_FARE = 5
 def initialize(origin_station)
 @origin_station = origin_station
 @destination_station = nil
 end
 def calculate_fare
 BASE_FARE + variable_trip_price
 end
 def complete_journey(destination_station)
 @destination_station = destination_station
 end
 private
 def zones_crossed
 (origin_station.zone - destination_station.zone).abs
 end
 def variable_trip_price
 (zones_crossed * PRICE_PER_ZONE).round(2)
 end
end

---

require 'oystercard'
describe Oystercard do
 let(:oystercard) { Oystercard.new }
 let(:oystercard_20) { Oystercard.new(20) }
 let(:orchard) { Station.new(1) }
 let(:somerset) { Station.new(2) }
 context "on creation" do
 it "is created with a balance of zero by default" do
 expect(oystercard.balance).to eq (0)
 end
 it "can be created with a balance" do
 oystercard = Oystercard.new(20)
 expect(oystercard.balance).to eq (20)
 end
 it "cannot be created with a balance greater than 50" do
 expect { Oystercard.new(51) }.to raise_error "Max balance is #{Oystercard::MAX_BALANCE}"
 end
 end
 context "top up" do
 it "can be topped up with a specific amount" do
 oystercard.top_up(20)
 expect(oystercard.balance).to eq (20)
 end
 it "will raise error if new balance would exceed 50" do
 oystercard = Oystercard.new(20)
 expect { oystercard.top_up(31) }.to raise_error "This would take you over the max balance!"
 end
 it "will not be topped up if new balance would exceed 50" do
 oystercard = Oystercard.new(20)
 expect { oystercard.top_up(31) }.to raise_error "This would take you over the max balance!"
 expect(oystercard.balance).to eq (20)
 end
 end
 context "tap in" do
 it "creates a new journey" do
 oystercard_20.tap_in(orchard)
 #expect new journey
 end
 it "raises error if user has unsufficient funds" do
 oystercard = Oystercard.new(0.5)
 expect { oystercard.tap_in(orchard) }.to raise_error "You don't have enough for this journey :("
 end
 end
 context "tap out" do
 it "charges the correct fare for a valid journey" do
 oystercard_20.tap_in(orchard)
 oystercard_20.tap_out(somerset)
 expect(oystercard_20.balance).to eq (17.80)
 end
 it "deducts the penalty fare if user did not tap in" do
 oystercard_20.tap_out(somerset)
 expect(oystercard_20.balance).to eq (15.00)
 end
 end
end

---

require 'journey'
require 'station'
describe Journey do
 let(:station_one) { Station.new(1) }
 let(:station_two) { Station.new(2) }
 let(:station_four) { Station.new(4) }
 context "validation" do
 it "is not valid without an origin" do
 expect { Journey.new }.to raise_error
 end
 end
 describe "complete" do
 context "calculates number of zones crossed" do
 it "from zone 1 to zone 4" do
 journey = Journey.new(station_one)
 journey.complete_journey(station_four)
 expect(journey.send(:zones_crossed)).to eq (3)
 end
 it "from zone 4 to zone 2" do
 journey = Journey.new(station_four)
 journey.complete_journey(station_two)
 expect(journey.send(:zones_crossed)).to eq (2)
 end
 it "from zone 4 to zone 4" do
 journey = Journey.new(station_four)
 journey.complete_journey(station_four)
 expect(journey.send(:zones_crossed)).to eq (0)
 end
 end
 context "calculates the variable fare" do
 it "from zone 1 to zone 4" do
 journey = Journey.new(station_one)
 journey.complete_journey(station_four)
 expect(journey.send(:variable_trip_price)).to eq (3.60)
 end
 it "from zone 4 to zone 2" do
 journey = Journey.new(station_four)
 journey.complete_journey(station_two)
 expect(journey.send(:variable_trip_price)).to eq (2.40)
 end
 end
 context "calculates the total fare" do
 it "from zone 1 to zone 4" do
 journey = Journey.new(station_one)
 journey.complete_journey(station_four)
 expect(journey.calculate_fare).to eq (4.60)
 end
 it "from zone 4 to zone 2" do
 journey = Journey.new(station_four)
 journey.complete_journey(station_two)
 expect(journey.calculate_fare).to eq (3.40)
 end
 end
 end
end

---

require "station"
describe Station do
 let(:station) { Station.new }
 context "zones" do
 it { is_expected.to respond_to(:zone) }
 it "defaults to Zone 1" do
 expect(station.zone).to eq 1
 end
 it "can be intialized with another zone" do
 station = Station.new(4)
 expect(station.zone).to eq 4
 end
 it "raises an error if the zone is greater than 5" do
 expect { Station.new(6) }.to raise_error "We only have 5 zones"
 end
 end
end

Answer 1

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 < MIN_BALANCE
end
def exceeds_max_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".

• Ben Hawker: Thank you Sergii, appreciate your time and completely get that there is no need to place the calculation of a` Journey's fare` in Oystercard. Will look to implement your suggestions later this week!