-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Kieran's Airport Challenge #2512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
require_relative 'plane' | ||
|
||
class Airport | ||
def initialize | ||
@plane_inventory = [] | ||
@capacity = 10 | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good practice to leave an empty line underneath each method. |
||
def land(plane) | ||
fail "Airport at capacity" if @plane_inventory.length >= @capacity | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider using SRP ie There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. improves readability |
||
@plane_inventory << plane | ||
"#{plane} landed" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
end | ||
|
||
def take_off(plane) | ||
fail 'No planes on the ground' if @plane_inventory.empty? | ||
@plane_inventory << plane | ||
"#{plane} plane(s) taken off and no longer in the airport" | ||
end | ||
def plane_inventory | ||
@plane_inventory.count | ||
end | ||
attr_reader :capacity | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice use of attr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
end | ||
|
||
|
||
|
||
# need another test to see if land_plane and take off correctly edit the plane inv log | ||
# if @plane_inventory += planes <= @capacity | ||
# else "UNSAFE TO LAND - Airport at capcity" | ||
# if (@plane_inventory += planes) > 10 | ||
# @plane_inventory = @capacity | ||
# puts "UNSAFE TO LAND - Airport at capcity" | ||
# end | ||
|
||
# plane = Plane.new | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
class Plane | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
require 'airport' | ||
require 'plane' | ||
|
||
describe Airport do | ||
|
||
it { is_expected.to be_kind_of(Airport)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good, you're clearly letting RSpec guide you first |
||
it { is_expected.to respond_to(:land).with(1).argument} | ||
it 'can land planes' do | ||
airport = Airport.new | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think argument specs are super useful, nice |
||
it 'can limit the number of planes that can land' do | ||
airport = Airport.new | ||
expect(airport.plane_inventory).to be <= airport.capacity | ||
end | ||
describe '#land' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clear outline of every function keeps things clarified, good practice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ie "decribe '#land' do" |
||
it 'raises an landing error when full' do | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: For this test, you can land planes 20 times, and then keep your same expectation to check that the error is raised. |
||
end | ||
end | ||
it 'raises an take off error when empty' do | ||
airport = Airport.new | ||
expect {airport.take_off(Plane.new)}.to raise_error "No planes on the ground" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
require 'plane' | ||
|
||
describe Plane do | ||
it { is_expected.to be_kind_of(Plane)} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SO THIS is how you split up files, gg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didn't know