Had my code review today for the Ruby BDD airport-challenge and I'm pretty pleased about it. My new coach is not known for handing out complements so in light of that I think I can breathe a sigh of relief. I am very glad I actually spent Sunday night revamping my tests and code, had I not done that today would have had a very different outcome.

Hey, Sorry to be quite harsh. Despite what it sounds like this is really good. You just need to neaten it up a bit and it would be perfect. I'm impressed with some of your code but just try to make sure that everything is consitant (sic) and has purpose. Really good :checkered_flag:

Full feedback after the break.

def locate_plane plane

can you make the name of this method express whats its doing, def location_of plane

def weather set_weather = %w(sunny stormy).sample

this is very imagintive. I would say theres too much logic in the defaulted value. essentailly you want this to either return whats its entered or a random [sunny, stormy]. It took me a couple of reads to see this when you could easily express this in the method body. I would encourage you to simplify this as it's a huge code smell.

def weather_good
def can_land

this is a boolean method so should be can_land?

def land(*)

this splat is hugely worrying, is it even needed?

def takeoff
  fail 'already flying' if @status == 'flying'

I would change @status == 'flying' to flying?

context 'can order a plane to take off' do
  it  is_expected.to respond_to :order_plane_takeoff 

I would always have the longer format of testing and never this it .. format. it "descriptive name" do .. end is far far better

it 'knows when a plane is in the airport' do
  plane = double :plane, location: 'airport'

great use of doubles, I would put this as a let(:plane)double :plane ... at the top though as you're using it a few times.

context 'traffic control' do
  it 'a plane cannot land if the airport is full' do
    Airport.any_instance.stub(:can_land).and_return(false)

this is old syntax. if airport was a let at the top like so let(:airport)Airport.new then you can simple say allow(airport).to receive(:canland).andreturn true this is the most recent syntax. But otherwise really good :)

plane = Plane.new

this is a massibve (sic) code smell. Make this a let so that its a different plane for each test. this will force you to write better tests. Right now they only work in sequence which will only cause complications later on. You always always always want every test to be able to run independently.

it 'responds to \'land\'' do
  expect(plane).to respond_to :land

all this tests is that there is a method called land. Whilst this is ok you can get too wins by testing the return value or expected change. Passing this test give you no functionality.

it 'responds to \'land\'' do
  expect(plane).to respond_to :land
end
it 'has a status of \'landed\' after landing' do
  plane.land :airport
  expect(plane.status).to eq 'landed'
end
it 'does not respond to \'land\' after landing' do
  expect  (plane.land :airport) .to raise_error 'already landed'
end
it 'responds to \'takeoff\'' do
  expect(plane).to respond_to :takeoff

again all of these respond_to tests are a waste. They're easy to pass and test nothing. You're better of testing that this method is actually doing.

I've got a lot of fixing to do but this is great, I've learnt a lot from this process and can't wait until my next code review.