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.
1(defn update [{:keys [key] :as params}]
2 (let [field (finders/find key :field)]
3 (if field
4 (let [validation-errors (validate-field (merge field params))]
5 (if (empty? validation-errors)
6 (save-and-redirect-to-topic (merge field params))
7 (redirect-for-validation-failure
8 (:topic-key field)
9 validation-errors)))
10 (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.
1(defn update [{:keys [key] :as params}]
2 (let [field (finders/find key :field)]
3 (if (nil? field)
4 (redirect-for-error "Could not find this field")
5 (let [validation-errors (validate-field (merge field params))]
6 (if (empty? validation-errors)
7 (save-and-redirect-to-topic (merge field params))
8 (redirect-for-validation-failure
9 (:topic-key field)
10 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.
1(defn update [{:keys [key] :as params}]
2 (let [field (finders/find key :field)]
3 (if (nil? field)
4 (redirect-for-error "Could not find this field")
5 (let [validation-errors (validate-field (merge field params))]
6 (if (seq validation-errors)
7 (redirect-for-validation-failure
8 (:topic-key field)
9 validation-errors)
10 (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.
1(defn update [{:keys [key] :as params}]
2 (let [field (finders/find key :field)]
3 (if (nil? field)
4 (redirect-for-error "Could not find this field")
5 (if (seq (validate-field (merge field params)))
6 (redirect-for-validation-failure
7 (:topic-key field)
8 (validate-field (merge field params)))
9 (save-and-redirect-to-topic (merge field params))))))And the payoff, flattening to a cond:
Step 4.
1(defn update [{:keys [key] :as params}]
2 (let [field (finders/find key :field)]
3 (cond (nil? field)
4 (redirect-for-error "Could not find this field")
5 (seq (validate-field (merge field params)))
6 (redirect-for-validation-failure
7 (:topic-key field)
8 (validate-field (merge field params)))
9 :else
10 (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.
1(defn update [{:keys [key] :as attributes}]
2 (let [field (finders/find key :field)
3 updated-field (merge field attributes)]
4 (cond (nil? field)
5 (redirect-for-error "Could not find this field")
6 (seq (validate-field updated-field))
7 (redirect-for-validation-failure
8 (:topic-key field)
9 (validate-field updated-field))
10 :else
11 (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.
1(defn update [{:keys [key] :as attributes}]
2 (let [field (finders/find key :field)
3 updated-field (merge field attributes)
4 validate-field (memoize validate-field)]
5 (cond (nil? field)
6 (redirect-for-error "Could not find this field")
7 (seq (validate-field updated-field))
8 (redirect-for-validation-failure
9 (:topic-key field)
10 (validate-field updated-field))
11 :else
12 (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.
1(defn update [{:keys [key] :as attributes}]
2 (let [field (finders/find key :field)
3 updated-field (merge field attributes)
4 validation-errors (delay (validate-field updated-field))]
5 (cond (nil? field)
6 (redirect-for-error "Could not find this field")
7 (seq @validation-errors)
8 (redirect-for-validation-failure
9 (:topic-key field)
10 @validation-errors)
11 :else
12 (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.
