4
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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

\$\endgroup\$
1
  • \$\begingroup\$ 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! \$\endgroup\$
    – Ben Hawker
    Feb 2 '16 at 3:10

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.