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

Russell's Airport Challenge #2504

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Russell's Airport Challenge #2504

wants to merge 2 commits into from

Conversation

Rmorbey
Copy link

@Rmorbey Rmorbey commented Apr 24, 2022

Russell Morbey

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).

  • User story 1: "I want to instruct a plane to land at an airport"
  • User story 2: "I want to instruct a plane to take off from an airport and confirm that it is no longer in the airport"
  • User story 3: "I want to prevent landing when the airport is full"
  • User story 4: "I would like a default airport capacity that can be overridden as appropriate"
  • User story 5: "I want to prevent takeoff when weather is stormy"
  • User story 6: "I want to prevent landing when weather is stormy"

README checklist

Does your README contains instructions for

  • how to install,
  • how to run,
  • and how to test your code?

Here is a pill that can help you write a great README!


attr_reader :capacity

def initialize(capacity = DEFAULT_CAPACITY)

Choose a reason for hiding this comment

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

I think you can just give whatever value you want default to be here, like capacity = 1, without needing to set the DEFAULT_CAPACITY variable. As with all of my comments, apologies if I'm entirely wrong....


def take_off(*)
fail "Flight cancelled due to bad weather" if bad_weather?
@airport.pop

Choose a reason for hiding this comment

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

I guess .pop could be tricky if we had multiple planes in the airport, as it'll always just pop the most recent entry to the airport array. FWIW, I used .delete(plane)

end

def weather
@weather ||= Weather.new

Choose a reason for hiding this comment

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

Oo, nice - ||= is new to me

@@ -0,0 +1,13 @@

class Weather

Choose a reason for hiding this comment

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

Oh, nice - I didn't break weather into a separate class, but definitely should have. Good compartmentalisation

forecast >= 6
end

private

Choose a reason for hiding this comment

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

As an open question, how come you used a private method here? I don't really understand private methods, so would be great to know


it 'instructs a plane to land at an airport' do

# User story 1

Choose a reason for hiding this comment

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

This is a nice way to make clear what you're testing. Didn't even occur to me

# To ensure safety
# I want to prevent landing when the airport is full

subject.capacity.times { subject.land Plane.new }

Choose a reason for hiding this comment

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

Like the use of the capacity variable here. Pretty sure I hardcoded a value

# I want to instruct a plane to land at an airport

# Arrange
airport = Airport.new

Choose a reason for hiding this comment

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

FWIW, you can use subject instead of declaring airport = Airport.new each time, when the describe for the test gives the class name

# Arrange
airport = Airport.new
stormy_weather = Weather.new
plane = Plane.new

Choose a reason for hiding this comment

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

FWIW, you can do let(:plane) { Plane.new } underneath describe Airport do, then you don't have to do plane = Plane.new inside each test

Copy link
Author

Choose a reason for hiding this comment

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

Super helpful cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants