Airport Challenge completed pull request #2509
Conversation
Describe #land to take argument Describe Plane class Describe land and expect it to land plane Complete user story 1
Describe Airport::TakeOff Expect Airport to not contain plane after it has taken off Complete user story 2
Complete user story 3
Add capacity and refactor to use it Refactor to use private methods Complete user story 4
Track and change landed state of plane Refactor tests to read more clearly Refactor plane spec to test behaviour not state Refactor plane to use private methods Refactor airport to use private methods Refactor airport_spec to test behaviour not state Add edge case tests for plane state
Add weather class and stormy? method Refactor weather to use private method Complete user story 5 Complete user story 5
Update program description for Add badges Add test results
There was a problem hiding this comment.
Well this is really great work — it's true you've pushed the SRP to its extremes here, so one method is really doing one thing, but it's good for you to experiment with this. In real situations there would likely be tradeoffs and other things to consider, so it's good to have a think about what is usually to be prioritised when it comes to splitting code into smaller chunks. When it comes to edge cases, it's very hard (sometimes impossible) to cover everything in more complex projects, so again it's a matter of prioritising - here most cases seem covered. Re: context in specs, there are tools the library can provide for us to write more concise setup code and tests, such as shared context, shared examples, etc - but the key is often to name things explicitly, and call helper methods, etc, explicitly as well, rather than relying too much on implicit context
Let me know if you have any further questions about this feedback — keep up the good work!
| take_off_plane(plane) | ||
| end | ||
|
|
||
| def include?(plane) |
There was a problem hiding this comment.
If your method include? doesn't have to be part of the public methods (i.e the caller of the class is not going to need these methods), you could set it as a private one
| @@ -0,0 +1,27 @@ | |||
| class Plane | |||
| def initialize | |||
| @landed = nil | |||
There was a problem hiding this comment.
It might be good to set a default value that makes sense for @landed - since it's a boolean, setting it as an initial value to nil might cause unexpected behaviour down the line (e.g what if we'd call .landed? when this attribute is still nil)
| describe Airport do | ||
| let(:airport) { Airport.new } | ||
| let(:plane) { instance_double('Plane', land: true, take_off: false) } | ||
| before(:each) { allow(airport).to receive(:stormy?).and_return(false) } |
There was a problem hiding this comment.
Rather than stubbing the method stormy? on Airport (which is private), a slightly better solution would be to stub it on a Weather instance double which would be injected into Airport - so you can replace the default implementation of the Weather with this double, and stub its behaviour
| end | ||
|
|
||
| def land_plane | ||
| airport.land(plane) |
There was a problem hiding this comment.
For these helper methods, I'd suggest moving them all to the same place (maybe the beginning of the file), and having them receive arguments rather than making use of the surrounding context (which is implicit), e.g:
def land_plane(airport, plane)
airport.land(plane)
end| context 'when flying' do | ||
| it 'should land' do | ||
| expect(plane).to receive(:land).and_return(true) | ||
| plane.land |
There was a problem hiding this comment.
This specific test doesn't really test for something, as we're asking to verify that the method plane.land will be called, just before calling it (so our test asserts something that it does by itself)
To assert that the method worked correctly, we could think of a way to verify the landed attribute has been set to true — however this would mean exposing the internal state of our plane instance, rather than testing for its behaviour
In the end, since the only behaviour from this class is to raise an error in case the plane is already "landed", maybe this is what we should assert in this test — that an error would be raise if we try to land the plane a second time
There was a problem hiding this comment.
(which seems to be done in the test just below!)
Your name
Luke Storey
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!