POODR Recap, or How I Learned To Stop Injecting Dependencies and Love OO
I’ll keep this concise, in the spirit of less is more.
Reading POODR has been one of the most beneficial things I’ve done in my long, illustrious career as a programmer (clocking in at approx $("Feb 02 2015").timeago();). It’s been recommended to me since ~week 2 of my immersive program at Flatiron School, but I’m glad I put it off until I’d built some un-POODR-like apps of my own. That context helped me draw connections between the abstract principles discussed in the book and very real production code I’m working on now. So without further ado:
Main takeaways from POODR:
1. Think about code in terms of objects and messages, not procedures
As Chapter 2 emphasizes, classes (and methods, really) should have a single responsibility. If you find your class carrying multiple responsibilities, break each one into its own separate class. You should conceptualize a class as an object that passes and receives messages, not something that executes some long procedure. Objects that execute a procedure can find themselves performing multiple responsibilities.
One strong example of this is a Ruby Code Challenge feature I’m currently working on, conveniently written entirely in JavaScript. Its basic functionality is as follows:
- challenge has a question, answer, solution, and assertion
- user inputs answer (a string of code, like “my_array = [1,2,3]”)
- feature evaluates answer to get its return value
- feature evaluates solution to get its return value
- feature parses answer and solution into a boolean assertion, such as
assert_equal(answer, solution) - after evaluating, feature displays feedback to the user:
- “Correct” if assertion returns true
- Rspec-esque message if assertion is false (ex. “expect x to equal y”)
- Error message stack trace (abbreviated) if error
Below is the _evaluateRuby function. In this iteration, you can see it’s violating the single responsibility principle like crazy. It’s following a long, procedural chain of events. It’s calling external services (the repl.it js client). It doesn’t give two duck types about POODR.
It is rigid, immobile, and viscous - 3 of the 4 bad code qualities codified by Robert Martin. It is not transparent, reasonable, or exemplary - positive qualities outlined in POODR (as more or less the inverse of Martin’s negative qualities).
CodeChallenge.prototype = {
_evaluateRuby: function(){
var studentResponse = this.editor.getValue();
var testingLib = [ "really long string" ];
var rubyRepl = this.challengeHolder.replItConnection; // establish connection to repl.it client
return rubyRepl.connect().then(function(){
return rubyRepl.evaluate( studentResponse, { // first call to repl.it api
stdout: function(out){}
});
}).then(function(result) {
if ( result.error.length ){
this.incorrectResponseFeedback(result.error); // first handoff to error handler
} else {
var assertion = this.parseAssertion(result.data);
rubyRepl.connect().then(function(){
return rubyRepl.evaluate( testingLib + "\n" + assertion, { // second call to repl.it api
stdout: function(out){}
});
}).then(function(result) {
if ( result.error.length ) {
this.incorrectResponseFeedback(result.error); // second handoff to error handler
} else {
this.responseHandler(result.data); // handoff to yet another function to display Correct/Incorrect response
}
}.bind(this));
}
}.bind(this));
},
// other functions, etc.
}
Applying what I learned from POODR, my first step in refactoring was to pull out each responsibility into its own separate object. rubyRepl.connect() and rubyRepl.evaluate() were moved into a new rubyRepl item that takes care of evaluating whatever code string you pass it. parseAssertion() was moved into its own rubyAssertion object, which builds a code string (specifically, the assertion code string, which reads something like assert_equal(studentResponse, solution)).
Now this function is closer to doing one thing: evaluating a string of Ruby code.
var W = require('when.js'); // https://github.com/cujojs/when
CodeChallenge.prototype = {
_evaluateRuby: function() {
var rubyRepl = this.challengeHolder.replItConnection; // establish connection to repl.it client
var studentResponse = this.editor.getValue();
return W(rubyRepl.evaluate( // first and only call to repl.it api
new RubyReplAssertion({ // new RubyAssertion object
response: studentResponse,
validation: this.validation
})))
.with(this)
.timeout(4000)
.then(function(response) {
if (response == 'true') { this.correctResponseFeedback(); }
else { this.incorrectResponseFeedback(response); }
})
.catch(W.TimeoutError, unresolvedResponseFeedback)
.catch(function(error) { this.incorrectResponseFeedback(error); });
},
// other functions, etc.
}
For the record, here is a full list of the code qualities discussed above:
Negative qualities (Robert Martin):
- Rigidity - parts are all attached; one change prompts many others
- Fragility - change one thing, unpredictable breaks
- Immobility - code hopelessly intertangled; reuse through duplication
- Viscosity - behaving badly is the most attractive option
Positive qualities (POODR):
- Transparent - see what will happen when you make a change
- Reasonable - cost of making a change is proportional to its value
- Usable - if you wrote it, you can use it in an unexpected context
- Exemplary - more like it is good; model for new hires
2. Managing dependencies is more important than keeping code DRY
On my first pass through this refactor, I spotted some code that I thought could use some DRY’ing up, specifically these two response feedback functions below:
CodeChallenge.prototype = {
correctResponseFeedback: function() {
this.clearResponseFeedback();
$( "<p class='correct-response'>Correct!</p>" ).insertBefore( this.answerBlock );
},
incorrectResponseFeedback: function(response) {
this.clearResponseFeedback();
$( "<p class='incorrect-response'>" + response + "</p>" ).insertBefore( this.answerBlock );
},
// other functions, etc.
}
Fairly similar, right? I saw enough overlap here to go ahead and refactor these into a single function, responseFeedback(), which could take any response message as an argument:
CodeChallenge.prototype = {
responseFeedback: function(response){
if (response == "Correct!") {
var pclass = "<p class='correct-response'>"
} else {
var pclass = "<p class='incorrect-response'>"
}
$( pclass + response + "</p>" ).insertBefore( this.answerBlock );
}
// other functions, etc.
}
Anyone who’s read Chapter 2 of POODR will recognize what’s wrong here: checking our response variable against a very brittle string value. Code should depend on variables that are unlikely to change, and there are few things more likely to change that a string value. If we decide to make as small a change as removing the !, our entire feature would break.
The original, less-DRY code is actually better in this respect, because it’s less brittle and more abstract.
As Sandi Metz has said, it’s a rookie mistake to sacrifice robustness for DRYer code. Novices look for duplication first, because that’s all they know how to recognize. In this way, teaching newbies to keep their code DRY is a form of training wheels, something simple they can hold on to until they understand how to manage dependencies through abstraction and encapsulation.
In general, it’s better to leave your code non-DRY than to introduce dependencies. It’s always easier to go back and fix duplication, than to try to refactor bad abstraction.
3. Reduce the cost of changing your code
The whole point of following the principles outlined in POODR is to reduce the cost of changing your code. And I can testify first hand that at least for this specific refactor, POODR worked like a charm.
It took two full afternoons of pairing with an experienced developer to clean up the non-OO javascript code for this code challenge feature. Once we’d applied some better dependency management and separated out responsibilities into their own methods and objects, it took me about an hour to add a new piece of functionality (one that had been floating around our issues backlog for months). Those are some damn fine results.
