There is a scene in the movie The Birdcage where the son tells his father that he (the son) has met a girl and is going to get married. The father begins gulping down the glass of wine that he has in hand. The son asks, Are you upset?
The father finishes the glass of wine and says, Let me tell you why.
Here is a function that I wrote several years ago.
(let ((dragging (dragging item)))
(when dragging
(let ((dx (- xx (car dragging)))
(dy (- yy (cdr dragging))))
(incf (offset-x item) dx)
(incf (offset-y item) dy))
(let ((pp (parent item)))
(when pp
(when (< (width pp) (+ (offset-x item) (width item)))
(setf (offset-x item) (- (width pp) (width item))))
(when (< (height pp) (+ (offset-y item) (height item)))
(setf (offset-y item) (- (height pp) (height item))))))
(when (< (offset-x item) 0) (setf (offset-x item) 0))
(when (< (offset-y item) 0) (setf (offset-y item) 0))
t)))
This is awful! Am I upset? Let me tell you why.
Is it the Single Responsibility Principle (SRP)? No.
Is it Don’t Repeat Yourself (DRY)? No.
Is it Mixing Levels of Abstraction? Closer, but not quite.
Those are all clearly violated by this code. But, that’s not really the problem. The problem is Why
. Nothing about this code tells you why it is here or what is doing.
There is no way to glance at that function and have any idea what’s going on. You have to read it carefully. You have to understand things that aren’t even in this source file to make head nor tail of it. Once you understand the second LET block, you will have nine more lines of code without the least inkling of why there should be nine more lines of code. Anyone care to hazard a guess as to why this function returns T (only) when we’re dragging?
Encapsulation
Two years ago, a colleague and I were tasked with providing docstrings for every function in all of the code we’d written in the last year. We’d done well on providing docstrings to the outward-facing functions, but now we had to do the rest. He started at one end of the directory (in alphabetical order), and I started at the other end. This gave me a good opportunity to look closely at a boat-load of code he’d written that I’d never really delved into before.
He was absolutely religious about encapsulating containers. If he had a hash-table or a p-list or a flat list in a DEFVAR, there was one and only one function that retrieved items from it and at most one function that added items to it. Those functions were one or two lines each (two if they needed a mutex). Those functions were named after what the collection was storing not what mechanism was used to store them.
A lot of times when people talk about the value of encapsulating, they talk about shielding the rest of the code from the implementation details so that if you need to replace how it’s actually implemented on the back end you can do it without breaking any existing code. You are protecting your precious implementation from how people will use it so that you can someday replace the implementation with an even more precious implementation next year (when your language finally gets first-class functions).
I’ve been coding for a good long time now. I’m going to let you in on a little secret. Code almost never gets replaced. When code does get replaced, it almost never continues to adhere to the old API (there was always a semantic leak). If there is a business justification strong enough to let you replace the code, it’s because the old code has become an unmaintainable mess of people subverting the interface or the code as it is didn’t scale and now synchronous things need to happen asynchronously or local things have to happen remotely and hiding that under your old API isn’t going to relieve the bottlenecks.
Trying to insulate your code so that it’s easy to replace is looking down the wrong end of the telescope. The real benefit of encapsulation is that the people who read your code later can be half-asleep and still get everything—your code will scream its meaning. The real benefit of encapsulation is that the person debugging your code can set a break-point in a place that means something—not in the seventeen places the state might have changed but in the only place it could change.
Making It Better
Any ideas what the body in this function does?
(if (being-dragged-p item)
(handle-event ()
(let ((dx (- xx (drag-starting-x item)))
(dy (- yy (drag-starting-y item))))
(translate-widget item dx dy)
(keep-widget-inside-parent item)))
(ignore-event ())))
The new functions BEING-DRAGGED-P, DRAG-STARTING-X, and DRAG-STARTING-Y are just wrappers around what had been explicitly treated as an (OR NULL (CONS INTEGER INTEGER)).
(dragging item))
(defun drag-starting-x (item)
(car (dragging item)))
(defun drag-starting-y (item)
(cdr (dragging item)))
It is still an (OR NULL (CONS INTEGER INTEGER)) but nobody ever has to care. Nobody ever has to try to remember what the integers mean. Sure, you could replace it with a structure or a complex number, but why would you ever bother? Why would you ever look at it again?
The new macros HANDLE-EVENT and IGNORE-EVENT encapsulate the return value of this function into something with meaning.
`(prog1
(values t)
,@body))
(defmacro ignore-event (() &body body)
`(prog1
(values nil)
,@body))
It might still be too easy to write an event-handler with a path which doesn’t end in one of these two macros, but it is way better than that dangling T was. It looks like it’s really supposed to be there, and it looks like what it means rather than what it is.
The TRANSLATE-WIDGET and KEEP-WIDGET-INSIDE-PARENT functions can benefit greatly with some further helper functions (and analogous functions for top and bottom):
(offset-x item))
(defun (setf left) (x item)
(setf (offset-x item) x))
(defun right (item)
(+ (left item) (width item)))
(defun (setf right) (x item)
(setf (offset-x item) (- x (width item))))
Some Rules of Thumb
If you find that when you want to check (PRED1 ...) you instead have to check:
(PRED1 ...))
Then you should consider making a function that does them both. Consider the difference between these two blocks of code:
(connectedp (player2 g))
(not (pausedp g)))
...)
(when (game-active-p g)
...)
If you find that you are depending on the NULL-ness or positiveness or some other property of some number of state variables to decide which course of action to take, then you should consider making predicates named after your state. In many OO scenarios, you may even want to explicitly track (or calculate) which state you are in at all times.
`(ecase (calculate-or-fetch-state-of g)
,@clauses))
(state-case g
(:pause-screen-showing
...)
(:settings-menu-showing
...))
In more imperative languages, it may even be beneficial to keep a STATE member variable in your class. When doing that, make sure that there is one and only one function which actually mutates the value of that STATE member. This will let you:
- Log all state transitions without having to hunt for all of them.
- Quickly hunt for all of them if you want to do that
- Set a break point on all state changes.
- Enforce the validity of transitions (or at least scream loudly when something transitions from STOPPED to PAUSED without having passed through PLAYING first).
If you have to check whether some resource is being used by some instance, don’t ask it which resource it is using, ask it whether it is using the one you want.
;;; that sockets are comparable with #'=
(loop :for player :in all-networked-players
:until (= socket-with-something-happening
(player-socket player))
:finally (return player))
;;; Better: All I wanted to know is, "Is this yours?"
(loop :for player :in all-networked-players
:until (player-using-socket-p player
socket-with-something-happening)
:finally (return player))
Encapsulation is about protecting the person who has to read your code. It’s not about protecting your code.