-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Tom O'Neill airport challenge #2519
base: main
Are you sure you want to change the base?
Conversation
end | ||
|
||
it 'Will not do so if the airport is full' do | ||
Airport::CAPACITY.times { subject.land(Plane.new) } |
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.
Good use of constant instead of magic number
|
||
describe '#depart' do | ||
it 'Can instruct a stored plane to take off from the airport' do | ||
plane = Plane.new |
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.
Since you are initialising a plane multiples times in the spec, you could consider using let to define a plane variable and then refer to that
let(:plane) { Plane.new }
expect(subject.stored_planes).to eq [] | ||
end | ||
|
||
it 'Will not do so if there are no planes in the airport' do |
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.
Could reword the it statement to be less wordy and easier to read, perhaps by using should ( e.g. 'should not depart if there are no planes') or consider a context block to set the stage for multiple tests where the airport is empty.
@@ -0,0 +1,46 @@ | |||
require_relative 'plane' |
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.
Believe if you require the files in the spec_helper you don't need to require them on each individual file as well
|
||
def land(plane) | ||
fail 'Airport full!' if full? | ||
fail 'Cannot land - stormy weather!' if @weather == 'Stormy' |
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.
Consider refactoring the condition into it's own method (SRP)
end | ||
|
||
def empty? | ||
@stored_planes == [] |
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.
You could use .empty? which might be cleaner
|
||
def set_weather | ||
die = rand(100) | ||
die > 100 ? 'Stormy' : 'Sunny' |
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.
Consider if there is a way to write this without the ternary
Tom O'Neill
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).
README checklist
Does your README contains instructions for
Here is a pill that can help you write a great README!