Kieran's Airport Challenge #2512
Conversation
| @@ -0,0 +1,35 @@ | |||
| require_relative 'plane' | |||
There was a problem hiding this comment.
SO THIS is how you split up files, gg
| @capacity = 10 | ||
| end | ||
| def land(plane) | ||
| fail "Airport at capacity" if @plane_inventory.length >= @capacity |
There was a problem hiding this comment.
consider using SRP ie
@plane_inventory.length >= @capacity
could equal just 'full?' if you define below
| def plane_inventory | ||
| @plane_inventory.count | ||
| end | ||
| attr_reader :capacity |
There was a problem hiding this comment.
Do you need the capacity variable to be visible outside of this class? If not, and if this is only so you can test it, it's worth removing it - testing the capacity variable will come indirectly by testing its behaviour, ie. if you land 20 planes, landing a 21st will result in an error.
|
|
||
| describe Airport do | ||
|
|
||
| it { is_expected.to be_kind_of(Airport)} |
There was a problem hiding this comment.
good, you're clearly letting RSpec guide you first
| plane = Plane.new | ||
| expect(airport.land(plane)).to eq("#{plane} landed") | ||
| end | ||
| it { is_expected.to respond_to(:take_off).with(1).argument} |
There was a problem hiding this comment.
I think argument specs are super useful, nice
| airport = Airport.new | ||
| expect(airport.plane_inventory).to be <= airport.capacity | ||
| end | ||
| describe '#land' do |
There was a problem hiding this comment.
clear outline of every function keeps things clarified, good practice
| def initialize | ||
| @plane_inventory = [] | ||
| @capacity = 10 | ||
| end |
There was a problem hiding this comment.
It's good practice to leave an empty line underneath each method.
| def land(plane) | ||
| fail "Airport at capacity" if @plane_inventory.length >= @capacity | ||
| @plane_inventory << plane | ||
| "#{plane} landed" |
There was a problem hiding this comment.
You don't need the String included here, as we can assume the plane has indeed landed if no error has been raised. This is maybe more important later on when we start working towards specifications / practice tech tests, but for now it can just help to try to keep your implementation as simple as possible.
| def plane_inventory | ||
| @plane_inventory.count | ||
| end | ||
| attr_reader :capacity |
There was a problem hiding this comment.
Do you need the capacity variable to be visible outside of this class? If not, and if this is only so you can test it, it's worth removing it - testing the capacity variable will come indirectly by testing its behaviour, ie. if you land 20 planes, landing a 21st will result in an error.
| # puts "UNSAFE TO LAND - Airport at capcity" | ||
| # end | ||
|
|
||
| # plane = Plane.new |
There was a problem hiding this comment.
It's good practice to remove any commented out code / notes before opening a PR - you can work any work in progress as such if you need to
| airport = Airport.new | ||
| plane = Plane.new | ||
| @plane_inventory = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 , 13, 14, 15, 16, 17, 18, 19, 20, 21] | ||
| expect {airport.land(plane)}.to raise_error "Airport at capacity" |
There was a problem hiding this comment.
Instead of altering the Airport object's state directly - the @plane_inventory variable, we could remove that direct access to better protect the internal state of the object, and you could land a plane 20 times manually.
Check out the .times method here:
https://www.rubyguides.com/ruby-tutorial/loops/
For this test, you can land planes 20 times, and then keep your same expectation to check that the error is raised.
Kieran Carty
Please write your full name here to make it easier to find your pull request.
User stories
Please list which user stories you've implemented (delete the ones that don't apply).
User story 1: "I want to instruct a plane to land at an airport"
User story 2: "I want to instruct a plane to take off from an airport and confirm that it is no longer in the airport"
User story 3: "I want to prevent landing when the airport is full"
User story 4: "I would like a default airport capacity that can be overridden as appropriate"
I didn't manage to complete anything to do with the Weather user stories
README checklist
Does your README contains instructions for
Here is a pill that can help you write a great README!