Skip to content

Instantly share code, notes, and snippets.

@billeisenhauer
Created May 12, 2014 14:47
Show Gist options
  • Save billeisenhauer/5844e96dd5b8e7f4c441 to your computer and use it in GitHub Desktop.
Save billeisenhauer/5844e96dd5b8e7f4c441 to your computer and use it in GitHub Desktop.
Illustrating effects of bloated protocols. Which class promotes God classes? Assume a HeatRegulator class collaborates to heat a room when needed.
class LeakyRoom
DESIRED_MIN_TEMPERATURE = 72
attr_accessor :temperature, :occupied
end
class Room
def heat_needed?
# Policy based upon temp and whether occupied.
end
private
attr_accessor :temperature, :occupied
DESIRED_MIN_TEMPERATURE = 72
private_constant :DESIRED_MIN_TEMPERATURE
end
@billeisenhauer
Copy link
Author

Very contrived example.

I just see Rails developers leave tons of things available in the public API, but I don't think they even realize it.

In this example, say the need to heat the room happens after this Room class has been initially built. Say temperature and the notion of being occupied were already there. Also, the desired temp constant. If they were public, this would enable someone to misplace the responsibility for determining if the room needed heating somewhere else.

A private API makes it more likely that the object that has the knowledge does the job.

As a tangential note, people sometime think I'm crazy because I indent methods and other things after the private declaration. I see no one ever adopt that style. I consider what's above the private declaration to be the protocol, the public API. When you are surfing through the class, you can tell when you are looking at protocol versus implementation details purely by the indentation level. Its a small thing, but works for me.

@ntl
Copy link

ntl commented May 12, 2014

Ah, interesting. I can see how LeakyRoom could grow unwieldy if it had more aspects to it than just checking to see if the heat needs to be turned on. When that happens, I'd likely extract an entire collaborator:

class Room
  attr_reader :temperature, :occupied

  def initialize(temperature, occupied)
    @temperature = temperature
    @occupied = occupied
  end

  def heat_needed?
    HeaterSwitch.new(self).on?
  end

  class HeaterSwitch
    DESIRED_MIN_TEMPERATURE = 72

     def on?
       # Policy based on temp and whether occupied
    end
  end
end

The preference of attr_reader over attr_accessor is a compromise that reduces the surface area of the public API just enough to make the object immutable. If I take away the getters, then HeaterSwitch can't collaborate with Room. In some cases, however, I won't expose readers at all. In that case, I won't declare private accessors, I'll just use ivars:

class Room
  def initailize
    @furniture = []
  end

  def add_furniture(furniture)
    return unless fits?(furniture)
    @furniture << furniture
  end
end

Here, I want to make sure access to the @furniture array is always guarded by a check to see if the furniture would fit.

I think I'm seeing two very valid styles here; 1. pare down the public API as much as possible vs. 2. preserve enough public API to introduce collaborators. For me, HeaterSwitch is my private API. And when I eventually need to change how we detect if the heater should be on, I've got a context to work with where things like DESIRED_MIN_TEMPERATURE are first class citizens.

Unfortunately, I've seen some God classes that had very tiny public APIs because they allowed too many features to flow through a humungous private API. I've also seen some cases of "too many objects." Both scenarios are preferable to original LeakyRoom case, though. I think the discipline of creating good object APIs is a bit of a lost art, sadly.

@ntl
Copy link

ntl commented May 12, 2014

Also, I've never seen a defense of the "indent your privates" before. Makes sense now!

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