Skip to content

Instantly share code, notes, and snippets.

@kkchu791
Created November 7, 2015 19:52
Show Gist options
  • Save kkchu791/3a606281986297803910 to your computer and use it in GitHub Desktop.
Save kkchu791/3a606281986297803910 to your computer and use it in GitHub Desktop.
class Charlie
def initialize(guitar)
@guitar = guitar
end
def play(guitar)
guitar.type == 'electric' ? guitar.sound.upcase : guitar.sound.downcase
end
end
class Guitar
attr_accessor :type
def initialize(manufacturer, color, type="electric")
@manufacturer, @color, @type = manufacturer, color, type
end
def sound
'twang'
end
end
g = Guitar.new('Gibson', 'blue')
c = Charlie.new(g)
p c.play(g)
#Method sound would make more sense to be in class Guitar because it's a guitar action.
#Added guitar to sound.upcase and sound.downcase so it could find the method in class Guitar.
#Attr_accessor :guitar is unnecessary because the guitar attributes were set properly with def initialize(guitar)
#Attr_accessor :manufacturer, :color are unnecssary because the attributes are not called from within class Charlie. The type attribute is called from class Charlie so it is needed.
@charliemcelfresh
Copy link

  1. yes
  2. The Guitar class should provide everything for a guitar object. I like to say that "an object should know everything about itself". So -- the logic for sound should all be in the Guitar class
  3. yes, but a lot of folks would make an accessor for :guitar, which I think is ok, except it would only have to be a reader. One very common mistake I see is that engineers make attr_accessors where they need only attr_readers or attr_writers
  4. sort of. As you'll see, we won't use any accessors from the Guitar class in the corrected solution here:
class Charlie
  attr_reader :guitar

  def initialize(guitar)
    @guitar = guitar
  end

  def play
    guitar.sound
  end
end

class Guitar
  def initialize(manufacturer, color, type="electric")
    @manufacturer, @color, @type = manufacturer, color, type
  end

  def sound
    s = 'twang'
    @type == 'electric' ? s.upcase : s.downcase
  end
end

But again, I feel that it is probably better to do as you suggest, and simply use the @guitar instance variable inside the Charlie class, as there is no need to access a guitar attribute from outside that class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment