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

Airport challenge #2496

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Empty file added lib/airport
Empty file.
43 changes: 43 additions & 0 deletions lib/airport.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
require_relative "../lib/plane.rb"
require_relative "../lib/weather.rb"

class Airport

attr_reader :airplanes
attr_accessor :capacity, :sunny
DEFAULT_CAPACITY = 30

def initialize(capacity=DEFAULT_CAPACITY)
@airplanes = []
@capacity = capacity
condition = Weather.new

Choose a reason for hiding this comment

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

I would maybe think about creating the Weather instance when the plane takes off/lands, rather than creating the weather the same time the airport is created. As the weather will most likely not be the same as it was the day the airport was created.

@weather = condition.sunny
end

def land_plane(airplane)
fail "Airport is full" if airplanes.length == capacity

Choose a reason for hiding this comment

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

Good :)

fail "Can't land as weather is stormy" unless sunny

Choose a reason for hiding this comment

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

I can't see the instance variable sunny relating to anything, should it be weather.sunny?

fail "Airplane is already here" if airplane.landed
airplane.landed = true
airplanes << airplane

Choose a reason for hiding this comment

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

This seems good!

end

def takeoff_plane(airplane)
fail "Weather Stormy cannot take off" unless sunny

Choose a reason for hiding this comment

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

Same comment as above regarding sunny instance variable

fail "Airplane is already in the sky!" unless airplane.landed
return airplane if check_plane(airplane) == airplane
fail "Airplane is not at this airport"
end

private

def check_plane(airplane)
airplanes.each_with_index do |check,index|
next unless check == airplane
airplanes.delete_at(index)
airplane.landed = false
return airplane
end
end

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

def initialize(status=true)
@landed = status
end

def landed?
landed

Choose a reason for hiding this comment

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

This seems good, you could probably add a method for take-off also, as the plane has two objectives, to land and take off.

end
end
11 changes: 11 additions & 0 deletions lib/weather.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class Weather
attr_reader :sunny

def initialize
rand(10) == 0 ? @sunny = false : @sunny = true
end

def sunny?
sunny

Choose a reason for hiding this comment

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

Maybe add a stormy method also, if it's not sunny, what is the weather?

end
end
93 changes: 93 additions & 0 deletions spec/airport_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@

require '../lib/airport.rb'
require '../lib/plane.rb'

describe Airport do
let(:airplane) {double :airplane, :landed= => false, landed?: false}
let(:weather) {double :weather, :sunny= => true, sunny?: true}

Choose a reason for hiding this comment

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

Good job using doubles :)


it "Land a plane at the airport and confirm" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
expect(subject.land_plane(airplane)).to include(airplane)
end

it "Land plane then take off" do

Choose a reason for hiding this comment

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

Maybe change the description to something like 'takes off plane after it has landed' or something similar. This description seems like you're testing the landing and the take off in the same test

subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
subject.land_plane(airplane)
allow(airplane).to receive(:landed).and_return(true)
expect(subject.takeoff_plane(airplane)).to eq airplane
end

it "Check plane is in sky and not landed" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)

Choose a reason for hiding this comment

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

You could maybe add a before each block at the start of your spec for this, as it seems to be used quite often, if you like

subject.land_plane(airplane)
allow(airplane).to receive(:landed).and_return(true)
subject.takeoff_plane(airplane)
expect(airplane).to_not be_landed

Choose a reason for hiding this comment

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

good :)

end

it "Make sure plane that as taken off is not at the airport" do

Choose a reason for hiding this comment

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

i think there is a typo in here, has instead of as

subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
subject.land_plane(airplane)
allow(airplane).to receive(:landed).and_return(true)
subject.takeoff_plane(airplane)
expect(subject.airplanes).to_not include(airplane)

Choose a reason for hiding this comment

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

good test

end

it "Prevent plane to take off if not sunny" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
subject.land_plane(airplane)
subject.sunny = false
error = "Weather Stormy cannot take off"
expect{subject.takeoff_plane(airplane)}.to raise_error error

Choose a reason for hiding this comment

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

You could also add the error message in this line. 'raise_error "Weather stormy cannot take off"' - rather than having this over two lines

end

it "Prevent airplane to land if not sunny" do
subject.sunny = false
error = "Can't land as weather is stormy"
expect{subject.land_plane(airplane)}.to raise_error error

Choose a reason for hiding this comment

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

same comment as above

end

it "Raise an error if the airport is full" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
Airport::DEFAULT_CAPACITY.times { subject.land_plane(airplane) }
expect{subject.land_plane(airplane)}.to raise_error "Airport is full"

Choose a reason for hiding this comment

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

good :)

end

it "Check to see if you can fill, remove then fill airport" do

Choose a reason for hiding this comment

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

This test description seems a little confusing

subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
Airport::DEFAULT_CAPACITY.times { subject.land_plane(airplane) }
allow(airplane).to receive(:landed).and_return(true)
subject.takeoff_plane(airplane)
expect(subject.airplanes).to include(airplane)
end

it "Overwrite default airport capacity to 30" do
expect(subject.capacity=30).to eq 30

Choose a reason for hiding this comment

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

You could also test a new instance of airport with capacity as paramater. I would also consider changing this number, as the default capacity is already set to 30

end

it "Raise error if plane already in sky and try takeoff" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
error = "Airplane is already in the sky!"
expect{subject.takeoff_plane(airplane)}.to raise_error error
end

it "Raise error if plane already tries to land when already at airport" do
subject.sunny = true
allow(airplane).to receive(:landed).and_return(false)
subject.land_plane(airplane)
allow(airplane).to receive(:landed).and_return(true)
error = "Airplane is already here"
expect{subject.land_plane(airplane)}.to raise_error error
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 '../lib/plane.rb'

describe Plane do

Choose a reason for hiding this comment

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

Here you could add some tests to see if the plane is in the air or if it's landed

end
16 changes: 16 additions & 0 deletions spec/weather_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require "../lib/weather.rb"

describe Weather do

let(:weather) {double :weather, :sunny= => true, sunny?: true}

it "Check weather = sunny" do
expect(weather).to be_sunny

Choose a reason for hiding this comment

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

good

end

let(:weather2) {double :weather, :sunny= => false, sunny?: false}
it "Check weather != sunny" do
allow(weather2).to receive(:sunny).and_return(false)
expect(weather2).to_not be_sunny
end
end