Michael C. Feathers
Notes by Jeremy W. Sherman, 14 Oct 2013.
Use effect sketches to figure out where to target your characterization tests prior to making changes. Big fan-in means a big bang for your test-writing buck.
- Prior to changing, write characterization tests to pin down existing behavior.
- Characterization test: A test that characterizes the actual behavior of a piece of code (186).
- Not necessarily sufficient to test just the methods we plan to change directly.
- Chase down effects (ripples).
- This chapter is about techniques for doing so.
- Bubbles with “what can change”, “directed arrow”, “what gets changed as a side effect”.
- Limiting the scope and complexity of effect sketches (locality) is a hallmark of good code.
- Focus on what we’re worried might change, then work backward to figure out what all affects the code we’re focused on.
- Work forward from our code. What breaks if we change the code wrong? This is how we will sense, and test for, unwanted change.
- Use this to write characterization tests.
- Still just drawing the same sketches! Sketching is mondo useful.
- Note: Don’t forget to look at sub- and superclasses of the code you’re going to hack on for even more side effect zaniness.
- Obvious: Return values
- Less obvious: Byref arguments that get mutated in-place.
- Even less obvious: Messing with global/static/common data.
- Know your language! Where do effects stop? (const keyword, sealed, private vs. protected vs. package vs. public)
- This is really our main tool. [Ed.: You might also consider using Graphviz for generating your effect diagrams. Complicated ones can be a pain to lay out by hand, while a graph layout algorithm can handle complicated graphs like a champ.]
- Ideally, your code base has its own internal logic that limits the ways effects spread. This lets you spend less time hunting for the crazy.
- Most such rules are of the “yeah, but that would be stupid, so no-one does that” variety, not anything formal.
- Removing duplicate code (by calling a helper method) creates effect sketches with a smaller set of terminal nodes.
A -> B; A -> C; becomes A -> B; A -> C; B -> C; because C now uses B after some refactoring. And testing C will pin down both A and B.
- Breaking encapsulation in order to be able to produce better explanatory tests is worth it: Test coverage beats encapsulation.
Third-party libraries will kill you. Stick your own API in the middle, either by directly wrapping the underlying API, or by extracting higher-level methods.
-
Can’t change binary blob libraries, so have difficulty harnessing their direct clients to tests. This makes changing legacy systems death-gripped onto a third-party API nearly impossible.
-
Also can’t refactor and grow the third-party API to suit our codebase. We’re stuck with it as the vendor left it, warts and bugs and all.
-
How do we fix this?
- Write a brief description of a chunk of mostly API call code.
- Separate out the responsibilities from that paragraph.
- Peel out the core logic and get its mitts off those API calls.
So that’s responsibility-based extraction.
The other approach: Skin and Wrap the API. One version of our API hits the third-party API; the other version can be all fakes. And we can improve our API!
Skinning is a great option when the API is limited. For a big, expansive API, you probably don’t want to skin & wrap.
Responsibility-based extraction is easy: Apply Extract Method till the cows come home. Good tooling makes this safe and painless.
- Down-side: We’ll likely still have a mix of our code and API calls, and we still might have trouble testing our code in that mix. But the higher-level code that calls our new API can be tested more readily.
This isn’t either–or; mix and match as appropriate.
Clear and focused communication across the team is key to ensuring we don’t lose (or regain) structure. Some techniques are iterated story-telling, naked CRC, and conversation scrutiny. Instead of structure, you get graft points, where people just keep stuffing the new features into the mammoth, ageless system that defies human comprehension.
Why does this happen to systems?
- Too complex for the amount of time you have to work on it.
- Too complex no matter how much time you have.
- Too much firefighting leaves no time for architecting.
Separate “architect” position only works if the architect is down on the shop floor adjusting the plans and also ensuring the code matches the plans.
- Better: Communicate architectural vision to everyone working on the project.
- Question and answer: “What is the system architecture?” Someone tries to describe using just a few concepts. Start with most important pieces.
- Prompt next: “Is that all?” Then next most important, and so on.
Requires simplifying for brevity and clarity; this is often the ideal architecture, though pragmatic concerns force another.
- Serves as a roadmap.
- Do this repeatedly, over time, and watch how things change.
- 1980s Hypercard - one card per class - write down Class Name, Responsibility, Collaborations
- Moved from CRC -> UML in 90s
- Naked CRC: Use index cards, but don’t write on them!
- How you lay them out, group them, overlap them, and what you say as you lay them and point at them, helps people visualize and understand the system.
- For example, can overlay collaborators, mirror layout client–server, show multiplicity, etc.
- Cards represent instances, not classes
- Overlap cards to show a collection of them
- Often very focused on the mechanics of changing a few mega-classes.
- Not at all looking to create new classes; just want to get in and get out.
- But listen to what you say you’re trying to do: You might have just identified a new class!
- If the code itself and the words you use to describe it differ markedly, have a think.
“There is something mesmerizing about large chunks of procedural code: They seem to beg for more” (224).
Note: You can use these same techniques when designing new systems. They’re not just good for legacy systems! See also: Object-Oriented Reengineering Patterns (Demeyer et. al., 2002). A whole bunch more techniques like these.
Guided by the single responsibility principle, apply sprout class and sprout method till things get sane. Use the interface segregation principle after if you can. Sussing out (hidden) responsibilities takes practice, but there are some techniques you can try now.
- Most features are little tweaks. Just stuff ’em in, right?
- Many little tweaks mean lots of code means bloated classes.
Big classes are bad because:
- Instance variable interactions compound.
- Changes all pile onto the same code, so losing orthogonality means losing task granularity (and blowing your deadline).
- Hard to test, because their gooey encapsulated data innards are so large that we can’t poke at them well enough via tests.
How do we make changes without making things worse?
- Sprout Class: New code in a new class.
- Sprout Method: New code in a new method.
- Finally, refactor, driven by the single-responsibility principle (SRP).
Single Responsibility Principle: Every class should have a single purpose in the system, and there should be only one reason to change it. (246)
Qualify that with “main purpose” to avoid extremism.
Find responsibilities by grouping method names. Can do this with instance and class variable names too.
- Group methods: Write down method names and access types. Group them.
- Look at hidden methods: If there are many private and protected methods, it’s likely there’s a new class dying to get out.
- Look for decisions that can change: Shifting change points into their own classes makes it easier for the architecture to adapt to change later without fracturing.
- Look for internal relationships: Are certain instance variables used by some methods and not others?
- Sketch out internal relationships: make feature sketches: add a node per ivar, then add a node per method (skip constructors often) and an arrow from the method to all the ivars it accesses or modifies. Look for clusters and circle them. There might be several possible clusterings; pick the one that works best design-wise.
- Note: Feature sketches are basically effect sketches where the arrows go the other direction (254).
- Look for the primary responsibility: Describe the class’s responsibility in a single sentence.
- Two kinds of SRP violation, one at the interface level, one at the implementation level. We care about implementation-level SRP violations: Is it a mega-class, or does it delegate out work to a network of smaller collaborators?
- Can resolve interface-level issues by guidance of Interface Segregation Principle.
Interface Segregation Principle (ISP): Split a class up so each grouping of methods is an interface. Clients can then depend on an interface rather than the entire beastly class of doom that implements them all. Clients also don’t have to be recompiled each time the class itself changes.
- When all else fails, do some scratch refactoring (212) and see how it shakes out.
- Scratch refactoring: Rename and rearrange and do whatever. Forget correctness. Look for design. Then, the most important step: Throw all your scratch changes out!
- Focus on the current work: Maybe whatever you’re trying to do right now is a new responsibility worth its own (substitutable) class.
- Get better by reading more about design patterns, and especially, more code (open source is a good place to get more code to read).
- How are the projects organized? How do they name things? Do the class names match the methods?
- You’ll learn to identify hidden responsibilities over time.
Strategy:
- Identify the responsibilities.
- Share that identification across the entire team.
- Break the class down along those lines only as needed. Real work can proceed alongside the refactoring.
Tactics:
- Sprout classes and have the implementation delegate to them.
- Straightforward if you can harness with tests.
- Riskier and slower otherwise (267–268):
- Group ivars.
- Copy out the methods you want to move into new methods with prefix MOVING. Don’t delete the old method; you want it in place in case there were dependencies on inheritance and overriding methods.
- See what breaks by leaning on the compiler. Watch out for shadowing and method overrides.
- Now separated within the class; push them out to a new class, stick an instance in the old class, and call through.
- Nuke the MOVING prefix.
Be careful not to be too ambitious with this extract class thing. What you have now works; after you shred it all apart, if you’re not supremely careful, it might not.
Aim for better, not perfect.