Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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

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

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

Source Link
Sergii K
  • 666
  • 4
  • 12

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

lang-rb

AltStyle によって変換されたページ (->オリジナル) /