Common issues

Use of magic numbers (e.g. on capacity)

def initialize
  @capacity = 6
end

6 is a numeric literal and its purpose in this statement is unclear. Encapsulate in a constant:

DEFAULT_CAPACITY = 6

def initialize
  @capacity = DEFAULT_CAPACITY
end

Use of strings instead of symbols

Each time a string literal (e.g. 'flying') is interpreted by Ruby, a new string object is created in memory. Therefore, every time a method is called that contains a string literal (e.g. 'sunny') a new object is created. This can lead to lots of unnecessary objects being created when we're not interested in the object identity of a string, just its value. To overcome this, use symbols instead e.g.: :flying, :sunny.

Command/Query naming and separation

Methods should be either commands or queries. As a general rule:

• Command methods should start with a verb: what does the method do?
• Query methods should be a nounal.
• Avoid naming query methods with a verb.
• Avoid naming command methods with a noun.
• Avoid depending on the return value of a command method (this rule is often broken!).
• Never have query methods that alter program state.

Defining attr_reader then accessing the instance variable directly.

attr_reader :capacity

def full?
  planes.count >= @capacity
end

Prefer delegating to the reader method (planes.count >= capacity) if it is defined, instead of accessing the instance variable.

Redundant initialization of instance variables

Instance variables are initialized when first assigned. The following is therefore redundant:

def initialize
 @name
end

And:

def initialize
  @name = nil
end

if / elsif with unrelated conditionals

if stormy?
  fail 'Bad weather'
elsif full?
  fail 'Airport full'
else
  ...
end

prefer separate methods for validation, or use guard clauses:

fail 'Bad weather' if stormy?
fail 'Airport full' if full?

...

Defining attr_accessor when you mean attr_reader

Defining attr_accessor then defining another mutator (do one or the other)

Reek Attribute Smell

class Plane
  attr_accessor :landed

  def land
    landed = true
  end
end

Which is the correct use of an instance of Plane?

plane.landed = true

or

plane.land

Prefer the custom method (land) for more control over the value of @landed and use attr_reader instead.

Using get_ and set_ prefixes on methods

This is a Java convention, but is disliked in Ruby. Use attr_* conventions instead: status over get_status and status= over set_status.

Ternaries for returning booleans

def stormy?
  weather == :stormy ? true : false
end

weather == :stormy is already a boolean expression, so the ternary is redundant. Use ternaries when you want to return values other than true and false. Otherwise, just return the boolean:

def stormy?
  weather == :stormy
end

Declaring a subject instead of using RSpec subject

describe Plane do
  let(:plane) { Plane.new }

  it 'can land'
    plane.land
    expect(plane).to be_landed
  end
end

Is the same as:

describe Plane do
  it 'can land'
    subject.land
    expect(subject).to be_landed
  end
end

Or, use a named subject:

describe Plane do
  subject(:plane) { Plane.new }

  it 'can land'
    plane.land
    expect(plane).to be_landed
  end
end

Use of block syntax in stubs instead of .and_return

plane = double :plane
allow(plane).to receive(:landed?) { true }

Use the block if you need to give the landed? stub some code, otherwise prefer:

plane = double :plane
allow(plane).to receive(:landed?).and_return true

Or use the shortcut syntax:

plane = double :plane, landed?: true

Use before blocks to set up objects rather than repeat code

Incomplete unit tests

describe Airport do
  let(:plane) { double :plane }

  it 'can land planes' do
    allow(plane).to receive(:land)
    subject.land plane
    expect(subject.planes).to include plane
  end
end

This only tests half of the requirements (depending on your design). We are not testing that the land method of plane is called:

describe Airport do
  let(:plane) { double :plane }

  it 'can land planes' do
    expect(plane).to receive(:land)
    subject.land plane
    expect(subject.planes).to include plane
  end
end

Does every implementation in your code have associated unit tests? For example, if you take off a plane:

def take_off(plane)
  ...
end

What if the airport has multiple planes? Do you test that the correct plane is no longer in the airport?

Mocking weather in feature tests with no stormy occurrence

In the grand finale, don't mock out the weather so that it is never stormy. Try to find ways to simulate a real run, but in such a way that your tests are not adversely affected by random behaviour. Ideally, this should be refactored into two separate tests.