jferg/familiar — pull request #1 by zachallaun (2013-07-29 22:00:03+00:00)
Title: Code review

Hey James,

This was fun to look at!

The actual changes I've made aren't particularly substantial, but they're examples of some simplifying refactorings I'd try to apply to the entire codebase.

High-level changes I'd recommend:

1. **Fewer threading macros**

My experience is that threading macros are great for you as a code-writer, but awful for someone else (or you, months later) as a code-reader. Since threading macros introduce implicit arguments, I have to mentally step through all the transformations, keeping track of the shape of the thing being threaded at every point. In contrast, "desugaring" a threading macro into a large `let` forces you to name each transformation, which is a big readability win. In general, I'd say that any use of a threading macro where the thing being threaded changes type is a code smell.

Here's an example of some code that I find quite difficult to read:

```clj
(defn times-matching
[var-vals trange]
(->> (select instance
(fields :time :value)
(with variable
(fields :name))
(where {:variable.name [in (keys var-vals)]
:time [between (map unparse-time trange)]}))
(group-by :time) ; assumes same time resolution!
vals
(map #(zipmap (map :name %) (map :value %)))
(filter #(= % var-vals))
count))
```

2. **Tests!**

Having at least semi-comprehensive test coverage makes refactoring 100x easier: `lein midje :autotest`, make some changes, see tests pass, move on. The project is still small enough that you could spend a day and get to this point, but you're closing in on the point-of-no-return when it's going to be a huge pain in the ass to add tests.

3. **Optional persistence**

This is partially related to testability, but it would also help significantly with interactive development. Basically, I think you want persistence to be an optional layer than you can flick off and on, but you should be able to do everything in-memory if you want to. Not only does this make it easier to construct test data-sets for development, but there are reasonable use-cases, e.g. significant amounts of historical data.