Skip to content
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

Kieran's Airport Challenge #2512

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions lib/airport.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require_relative 'plane'

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didn't know


class Airport
def initialize
@plane_inventory = []
@capacity = 10
end

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider using SRP ie
@plane_inventory.length >= @capacity
could equal just 'full?' if you define below

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improves readability

@plane_inventory << plane
"#{plane} landed"

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice use of attr

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

3 changes: 3 additions & 0 deletions lib/plane.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Plane

end
30 changes: 30 additions & 0 deletions spec/airport_spec.rb
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)}

Choose a reason for hiding this comment

The 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}

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear outline of every function keeps things clarified, good practice

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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:
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.

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
5 changes: 5 additions & 0 deletions spec/plane_spec.rb
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