December 2014 Common Issues

General Advice

• get started early
• play in irb
• don't spend the weekend revising and then come back to this
• make a start, get stuck, fail, then have a break, revise and then come back to it
• send pull requests early and often

Specific issues

• (maciejk77) gotta ensure class and file names match ...

• avoid using terms that sound like questions when setting instance variables (Munded)

  def has_landed
    @landed = true
  end

(rubocop suggests updating to land? which is the wrong suggestion - wonder if we can fix that)

• it's good practice to refer to instance variables like @planes with an attr_reader, so we can replace @planes << plane with planes << plane (GJMcGowan, PhoebeHugh)

• use private attr_writer method to set instance variables (massud, maxlweaver)

• watch out for unused methods (maxlweaver)

• (GJMcGowan) Don't need plane1 below - when the plane is in the array it will just be the same as plane. Ideally we should be failing with an error of our own when the plane is not in the array rather than leaving it to through a no method exception for nil when it is not present:

  def plane_take_off(plane)
    fail 'Airport Empty' if @planes.empty?
    fail 'Stormy Weather' if stormy_weather?
    plane1 = @planes.delete(plane)
    plane1.take_off!
    plane1
  end

• (GJMcGowan) also regarding above, better to call method just 'take_off'

• (GJMcGowan) you shouldn't really be declaring variables here - they should be inside a before block

feature 'Grand Finale: Good Weather' do
  airport = Airport.new
  planes = []
  6.times { planes << Plane.new }

• (GJMcGowan) ensure test description matches what test actually does:

    it 'can return the landed planes' do
      expect(subject.planes).to eq []
    end

• (GJMcGowan) this could use a predicate, e.g. expect(subject).to be_flying

expect(subject.flying?).to be true

(could rubocop detect the above for us?)

• (maxlweaver) avoid magic numbers - prefer to set at some kind of CONSTANT, e.g.

class Airport
  DEFAULT_CAPACITY = 200

• can have specs refer to constants to DRY out spec e.g. Airport::DEFAULT_CAPACITY

• (M1lena) airport unit test includes Plane.new --> Airport unit test should be independent of Plane

• (M1lena) commented code hanging around

• (M1lena) plane landing at airport does not update plane's status, similarly for takeoff

• (M1lena, Phoebe) missing grand finale spec