Lately I've been re-reading some of the classic books in the clean code / OOP genre and trying to apply their lessons with renewed energy and thoughtfulness. This post lays out a brief struggle and solution in applying a few of Martin Fowler's Refactorings to a bit of web app code in Clojure.
My pair and I found ourselves having written the following code for an internal application. It got the job done, but it seemed a bit distasteful to both of us.
Step 0.
(defn update [{:keys [key] :as params}]
(let [field (finders/find key :field)]
(if field
(let [validation-errors (validate-field (merge field params))]
(if (empty? validation-errors)
(save-and-redirect-to-topic (merge field params))
(redirect-for-validation-failure
(:topic-key field)
validation-errors)))
(redirect-for-error "Could not find this field"))))
Full disclosure: it looked even worse than this when we first got it passing. This is after a few iterations of Extract Function (Refactoring calls it Extract Method, but we call them functions in Clojure).
Diving in
There are a couple of problems here, but the problem that bothered us the most was the many levels of nesting. One option to fix it would be to extract a function from the business inside the let, but we couldn't think of a good name for what it would be, aside from maybe update, which we're already looking at. In an imperative language like Java or Ruby, we could use guard clauses to bail early on an error condition like the one the outer if expression is concerned with - this is called Replace Nested Conditional with Guard Clauses in the book. Clojure doesn't have a notion of "early returns" - you could certainly scrape something together if you really wanted, using a macro, but let's not get ridiculous.
Taking a step back, the problem we're trying to solve here is the nesting. Since we're not going to do guard clauses, maybe we can find another way to flatten these out. We can start by negating the outer expression to handle errors first. Maybe that would improve things?
Step 1.
(defn update [{:keys [key] :as params}]
(let [field (finders/find key :field)]
(if (nil? field)
(redirect-for-error "Could not find this field")
(let [validation-errors (validate-field (merge field params))]
(if (empty? validation-errors)
(save-and-redirect-to-topic (merge field params))
(redirect-for-validation-failure
(:topic-key field)
validation-errors))))))
I should mention here that we've got a good suite of unit tests in place, and running continuously, to help ensure us that the changes we're making are safe. Let's move the other error case, where validation fails, up as well. This way the error cases are closer together, the way we're thinking about them.
Step 2.
(defn update [{:keys [key] :as params}]
(let [field (finders/find key :field)]
(if (nil? field)
(redirect-for-error "Could not find this field")
(let [validation-errors (validate-field (merge field params))]
(if (seq validation-errors)
(redirect-for-validation-failure
(:topic-key field)
validation-errors)
(save-and-redirect-to-topic (merge field params)))))))
Our tests still pass after these changes, and the error cases are maybe a bit clearer, but the nesting hasn't flattened out at all yet. It's not clear at all to me that this is helping.
In order to flatten it out further, it'd be nice if we could just use a cond, putting all the cases at the same level, but we'd first need to do something about this local variable, validation-errors. Locals can be poison for refactoring, so Fowler suggests that Replace Temp with Query can be of some assistance. In our case, this would just entail using Inline Method to replace the validation-errors with (validate-field (merge field params)) wherever it occurs.
I was initially very hesitant to consider it, because I happen to know that validate-field hits the database. I remembered Fowler's advice that performance concerns could easily be handled by the object receiving the query, but didn't quite see how that helped me in Clojure. I could use memoization, but I didn't want to have to worry about returning stale answers on future web requests, consuming extra memory beyond this request, flushing the memoization cache on every request, or any of other complexity that typically comes with cache management. So it was a bit of a leap of faith that we'd sort out a way to solve the performance issues, but we forged on and replaced the local with its query. Perhaps you can already spot the solution?
Step 3.
(defn update [{:keys [key] :as params}]
(let [field (finders/find key :field)]
(if (nil? field)
(redirect-for-error "Could not find this field")
(if (seq (validate-field (merge field params)))
(redirect-for-validation-failure
(:topic-key field)
(validate-field (merge field params)))
(save-and-redirect-to-topic (merge field params))))))
And the payoff, flattening to a cond:
Step 4.
(defn update [{:keys [key] :as params}]
(let [field (finders/find key :field)]
(cond (nil? field)
(redirect-for-error "Could not find this field")
(seq (validate-field (merge field params)))
(redirect-for-validation-failure
(:topic-key field)
(validate-field (merge field params)))
:else
(save-and-redirect-to-topic (merge field params)))))
This is looking much nicer, provided we can find a way to actually make it work. While we're thinking hard about how to solve this, let's extract the updated field to the outermost local block (a merge is cheap enough that it doesn't bother me to do this extra work that the topmost error case won't need. And merging into nil is totally safe, thanks to the sequence abstraction. Let's also rename the argument to something more meaningful than params:
Step 5.
(defn update [{:keys [key] :as attributes}]
(let [field (finders/find key :field)
updated-field (merge field attributes)]
(cond (nil? field)
(redirect-for-error "Could not find this field")
(seq (validate-field updated-field))
(redirect-for-validation-failure
(:topic-key field)
(validate-field updated-field))
:else
(save-and-redirect-to-topic updated-field))))
Luckily, thinking through the memoization issues brought us to what I think is kind of an interesting solution that didn't have any of the big difficulties I'd been worried about, but still avoided the extra database call and allowed us to flatten out these conditionals:
Step 6.
(defn update [{:keys [key] :as attributes}]
(let [field (finders/find key :field)
updated-field (merge field attributes)
validate-field (memoize validate-field)]
(cond (nil? field)
(redirect-for-error "Could not find this field")
(seq (validate-field updated-field))
(redirect-for-validation-failure
(:topic-key field)
(validate-field updated-field))
:else
(save-and-redirect-to-topic updated-field))))
The first call of the inner validate-field function will produce a value, and that value will indeed be cached via Clojure's memoize functionality. However, the function we're memoizing is only ever bound to a local, and so once that local is out of scope, the memoized function (including its enclosed memoization cache) will be garbage collected. So we don't need to worry about memory leaks, stale values, or manually expiring this cache. Hopefully you can convince yourself that this memoized function is a bit like a tiny, short-lived object that lazy-initializes a field the first time it's queried, and returns the field's value on subsequent queries.
Update: A couple of smart Clojure folks have pointed out that we could have used delay here in place of memoize. That'd look like this:
Step 6a.
(defn update [{:keys [key] :as attributes}]
(let [field (finders/find key :field)
updated-field (merge field attributes)
validation-errors (delay (validate-field updated-field))]
(cond (nil? field)
(redirect-for-error "Could not find this field")
(seq @validation-errors)
(redirect-for-validation-failure
(:topic-key field)
@validation-errors)
:else
(save-and-redirect-to-topic updated-field))))
One nice outcome of using delay is that we can reinstate our original name, validation-errors, and we can treat it more like a local variable. Thanks to @ericnormand and @alanmalloy for bringing it up.
Reflection
This code seems much clearer to me than what we started with. At a glance I can see the three paths of execution, and I can more easily see these paths as peers, which matches my mental model of the decisions this function makes. It's actually 2 lines longer than the original, but I care way less about lines of code than ease of understanding.
There may not be anything earthshaking about this memoize-local-function technique - it's probably been used for the same purpose by others - but I'm certainly going to keep it in my back pocket for the next time a local appears to keep me from flattening nested conditionals. The bigger lesson I'm taking away is that even with advice that seems clearly irrelevant based on language details, there's often a way to solve the underlying issue - especially when you're working in a flexible and powerful language.
