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