One of the user stories I had to tackle in a recent sprint was to import data maintained by a non-technical colleague in a Google Spreadsheet into our analytics database. I quickly found a Java API for Google Spreadsheets that looked promising but turned out to be more tricky to get up and running than expected at first glance. In this article, I show you how to use this library from Clojure and avoid some of the pitfalls I fell into.

Google Spreadsheets API

The GData Java client referenced in the Google Spreadsheets API documentation uses an old XML-based protocol, which is mostly deprecated. We are recommended to use the newer, JSON-based client. After chasing my tail on this, I discovered that Google Spreadsheets does not yet support this new API and we do need the GData client after all.

The first hurdle: dependencies

The GData Java client is not available from Maven, so we have to download a zip archive. The easiest way to use these from a Leiningen project is to use mvn to install the required jar files in our local repository and specify the dependencies in the usual way. This handy script automates the process, only downloading the archive if necessary. (For this project, we only need the gdata-core and gdata-spreadsheet jars, but the script is easily extended if you need other components.)

#!/bin/bash

set -e

function log () {
    echo "$1" >&2
}

function install_artifact () {
    log "Installing artifact $2"
    mvn install:install-file -DgroupId="$1" -DartifactId="$2" -Dversion="$3" -Dfile="$4" \
        -Dpackaging=jar -DgeneratePom=true
}

R="${HOME}/.m2/repository"
V="1.47.1"
U="http://storage.googleapis.com/gdata-java-client-binaries/gdata-src.java-${V}.zip"

if test -r "${R}/com/google/gdata/gdata-core/1.0/gdata-core-1.0.jar" \
        -a -r "${R}/com/google/gdata/gdata-spreadsheet/3.0/gdata-spreadsheet-3.0.jar";
then
    log "Artifacts up-to-date"
    exit 0
fi

log "Downloading $U"
cd $(mktemp -d)
wget "${U}"
unzip "gdata-src.java-${V}.zip"

install_artifact com.google.gdata gdata-core 1.0 gdata/java/lib/gdata-core-1.0.jar

install_artifact com.google.gdata gdata-spreadsheet 3.0 gdata/java/lib/gdata-spreadsheet-3.0.jar

Once we’ve installed these jars, we can configure dependencies as follows:

(defproject gsheets-demo "0.1.0-SNAPSHOT"
  :description "Google Sheets Demo"
  :url "https://github.com/ray1729/gsheets-demo"
  :license {:name "Eclipse Public License"
            :url "http://www.eclipse.org/legal/epl-v10.html"}
  :dependencies [[org.clojure/clojure "1.8.0"]
                 [com.google.gdata/gdata-core "1.0"]
                 [com.google.gdata/gdata-spreadsheet "3.0"]])

The second hurdle: authentication

This is a pain, as the documentation for the GData Java client is incomplete and at times confusing, and the examples it ships with no longer work as they use a deprecated OAuth version. The example Java code in the documentation tells us:

// TODO: Authorize the service object for a specific user (see other sections)

The other sections were no more enlightening, but after more digging and reading of source code, I realized we can use the google-api-client to manage our OAuth credentials and simply pass that credentials object to the GData client. This library is already available from a central Maven repository, so we can simply update our project’s dependencies to pull it in:

:dependencies [[org.clojure/clojure "1.8.0"]
               [com.google.api-client/google-api-client "1.21.0"]
               [com.google.gdata/gdata-core "1.0"]
               [com.google.gdata/gdata-spreadsheet "3.0"]]

OAuth credentials

Before we can start using OAuth, we have to register our client with Google. This is done via the Google Developers Console. See Using OAuth 2.0 to Access Google APIs for full details, but here’s a quick-start guide to creating credentials for a service account.

Navigate to the Developers Console. Click on Enable and manage APIs and select Create a new project. Enter the project name and click Create.

Once project is created, click on Credentials in the sidebar, then the Create Credentials drop-down. As our client is going to run from cron, we want to enable server-to-server authentication, so select Service account key. On the next screen, select New service account and enter a name. Make sure the JSON radio button is selected, then click on Create.

Copy the downloaded JSON file into your project’s resources directory. It should look something like:

{
  "type": "service_account",
  "project_id": "gsheetdemo",
  "private_key_id": "041db3d758a1a7ef94c9c59fb3bccd2fcca41eb8",
  "private_key": "-----BEGIN PRIVATE KEY-----\n...\n-----END PRIVATE KEY-----\n",
  "client_email": "gsheets-demo@gsheetdemo.iam.gserviceaccount.com",
  "client_id": "106215031907469115769",
  "auth_uri": "https://accounts.google.com/o/oauth2/auth",
  "token_uri": "https://accounts.google.com/o/oauth2/token",
  "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
  "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/gsheets-demo%40gsheetdemo.iam.gserviceaccount.com"
}

We’ll use this in a moment to create a GoogleCredential object, but before that navigate to Google Sheets and create a test spreadsheet. Grant read access to the spreadsheet to the email address found in client_email in your downloaded credentials.

A simple Google Spreadsheets client

We’re going to be using a Java client, so it should come as no surprise that our namespace imports a lot of Java classes:

(ns gsheets-demo.core
  (:require [clojure.java.io :as io])
  (:import com.google.gdata.client.spreadsheet.SpreadsheetService
           com.google.gdata.data.spreadsheet.SpreadsheetFeed
           com.google.gdata.data.spreadsheet.WorksheetFeed
           com.google.gdata.data.spreadsheet.CellFeed
           com.google.api.client.googleapis.auth.oauth2.GoogleCredential
           com.google.api.client.json.jackson2.JacksonFactory
           com.google.api.client.googleapis.javanet.GoogleNetHttpTransport
           java.net.URL
           java.util.Collections))

We start by defining some constants for our application. The credentials resource is the JSON file we downloaded from the developer console:

(def application-name "gsheetdemo-v0.0.1")

(def credentials-resource (io/resource "GSheetDemo-041db3d758a1.json"))

(def oauth-scope "https://spreadsheets.google.com/feeds")

(def spreadsheet-feed-url (URL. "https://spreadsheets.google.com/feeds/spreadsheets/private/full"))

With this in hand, we can create a GoogleCredential object and initialize the Google Sheets service:

(defn get-credential
  []
  (with-open [in (io/input-stream credentials-resource)]
    (let [credential (GoogleCredential/fromStream in)]
      (.createScoped credential (Collections/singleton oauth-scope)))))

(defn init-service
  []
  (let [credential (get-credential)
        service (SpreadsheetService. application-name)]
    (.setOAuth2Credentials service credential)
    service))

Let’s try it at a REPL:

lein repl

user=> (require '[gsheets-demo.core :as gsheets])
nil
user=> (def service (gsheets/init-service))
#'user/service
user=> (.getEntries (.getFeed service
                              gsheets/spreadsheet-feed-url
                              com.google.gdata.data.spreadsheet.SpreadsheetFeed))
(#object[com.google.gdata.data.spreadsheet.SpreadsheetEntry 0x43ab2a3e "com.google.gdata.data.spreadsheet.SpreadsheetEntry@43ab2a3e"])

Great! We can see the one spreadsheet we granted our service account read access. Let’s wrap this up in a function and implement a helper to find a spreadsheet by name:

(defn list-spreadsheets
  [service]
  (.getEntries (.getFeed service spreadsheet-feed-url SpreadsheetFeed)))

(defn find-spreadsheet-by-title
  [service title]
  (let [spreadsheets (filter (fn [sheet] (= (.getPlainText (.getTitle sheet)) title))
                             (list-spreadsheets service))]
    (if (= (count spreadsheets) 1)
      (first spreadsheets)
      (throw (Exception. (format "Found %d spreadsheets with name %s"
                                 (count spreadsheets)
                                 title))))))

Back at the REPL:

user=> (def spreadsheet (gsheets/find-spreadsheet-by-title service "Colour Counts"))
user=>  (.getPlainText (.getTitle spreadsheet))
"Colour Counts"

A spreadsheet contains one or more worksheets, so the next functions we implement take a SpreadsheetEntry object and list or search worksheets:

(defn list-worksheets
  [service spreadsheet]
  (.getEntries (.getFeed service (.getWorksheetFeedUrl spreadsheet) WorksheetFeed)))

(defn find-worksheet-by-title
  [service spreadsheet title]
  (let [worksheets (filter (fn [ws] (= (.getPlainText (.getTitle ws)) title))
                           (list-worksheets service spreadsheet))]
    (if (= (count worksheets) 1)
      (first worksheets)
      (throw (Exception. (format "Found %d worksheets in %s with name %s"
                                 (count worksheets)
                                 spreadsheet
                                 title))))))

…and at the REPL:

user=> (def worksheets (gsheets/list-worksheets service spreadsheet))
user=> (map (fn [ws] (.getPlainText (.getTitle ws))) worksheets)
("Sheet1")

Our next function returns the cells belonging to a worksheet:

(defn get-cells
  [service worksheet]
  (map (memfn getCell) (.getEntries (.getFeed service (.getCellFeedUrl worksheet) CellFeed))))

This gives us a flat list of Cell objects. It will be much more convenient to work in Clojure with a nested vector of the cell values:

(defn to-nested-vec
  [cells]
  (mapv (partial mapv (memfn getValue)) (partition-by (memfn getRow) cells)))

We now have all the building blocks for the function that will be the main entry point to our minimal Clojure API:

(defn fetch-worksheet
  [service {spreadsheet-title :spreadsheet worksheet-title :worksheet}]
  (if-let [spreadsheet (find-spreadsheet-by-title service spreadsheet-title)]
    (if-let [worksheet (find-worksheet-by-title service spreadsheet worksheet-title)]
      (to-nested-vec (get-cells service worksheet))
      (throw (Exception. (format "Spreadsheet '%s' has no worksheet '%s'"
                                 spreadsheet-title worksheet-title))))
    (throw (Exception. (format "Spreadsheet '%s' not found" spreadsheet-title)))))

With this in hand:

user=> (def sheet (gsheets/fetch-worksheet service {:spreadsheet "Colour Counts" :worksheet "Sheet1"}))
#'user/sheet
user=> (clojure.pprint/pprint sheet)
[["Colour" "Count"]
 ["red" "123"]
 ["orange" "456"]
 ["yellow" "789"]
 ["green" "101112"]
 ["blue" "131415"]
 ["indigo" "161718"]
 ["violet" "192021"]]
nil

Our to-nested-vec function returns the cell values as strings. I could have used the getNumericValue method instead of getValue, but then to-nested-vec would have to know what data type to expect in each cell. Instead, I used Plumatic Schema to define a schema for each row, and used its data coercion features to coerce each column to the desired data type – but that’s a blog post for another day.

Code for the examples above is available on Github https://github.com/ray1729/gsheets-demo. We have barely scratched the surface of the Google Spreadsheets API; check out the API Documentation if you need to extend this code, for example to create or update spreadsheets.

OnyxMetail is hosting the next meetup of Cambridge NonDysfunctional Programmers next Thursday, 17th March. This month we’ll be taking a look at Onyx, a distributed cloud computing platform implemented in Clojure. We’re currently using Cascalog to process data on a Hadoop cluster, and are considering Onyx as a possible alternative. It will be interesting for us to hear what our local Clojure community makes of this new kid on the block.

One of my favourite talks at the recent Clojure Remote conference was Michael Drogalis’s keynote, where he discussed some of the principles behind Onyx’s data-driven API. At the Meetup, we’ll watch Michael’s Onyx talk from last year’s Clojure/conj. After the video, we’ll work through the getting started guide and tutorial together. Please see the Meetup page for full details.

I have been toying around with Clojure for five or six years now and whilst I really enjoy the way it allows me to think and to solve problems I find it difficult to come up with small but fun projects to work on to help me learn the language. I have spent many happy hours on sites such as the venerable project euler and more recently 4clojure and learned plenty but I don’t think either is as good a resource for problems which help you become a better coder as advent of code does because neither encourages as much refactoring.

OK, ok I've only done five so far..

Advent of code Christmas tree

The site has 25 days of questions and each one follows a similar pattern: (disclaimer: I haven’t completed all of them yet or even looked at them all so for all I know some of the later ones break the mould but I hope not).

  1. Explain some rules which typically will require you to parse text to numbers
  2. Provide some short examples which could be used as unit tests
  3. Provide access to a unique set of input to your problem

I will now show you as an example how the first problem helped me to write better code. (I have slightly exaggerated how bad some of my initial solutions were to highlight the learning and have given you full access to my inner monologue.)

Day 1

I’ve read the rubric and it is clear that I am going to need a function which converts a brace to a plus or minus one. In none of the examples do I see anything other than a ‘(‘ or a ‘)’ so I assume I can get away with the following:

(defn brace->movement
  [brace]
  (if (= "(" brace)
    1
    -1))

The first functional programming I did was some ML 16 or 17 years ago so I leap to recursion to solve the rest of the problem.

(defn calculate-floor
  [braces floor]
  (if (empty? braces)
    floor
    (let [h (first braces)
      m (brace->movement h)
      f (+ floor m)]
      (calculate-floor (rest braces) f))))

I start testing …

user> (calculate-floor "(())" 0)
-4
user> (calculate-floor "()()" 0)
-4

well this is not going well, both of these should have resulted in ‘0’. It looks like something must be going wrong with my brace->movement parsing function so I will just test that at the REPL. (I guess I should have done that first huh?)

user> (brace->movement "(")
1
user> (brace->movement ")")
-1

Hmmm.. nope, not that. Everything working perfectly. OK something other than ‘(‘ or ‘)’ must be being passed to that function. It must be first that is the problem:

user> (first "(())")
\(

Ah ha! Of course a string is a sequence of characters and if I call first on a sequence of characters it must give me a character. So now do I go for a function that parses a character such as:

(defn brace->movement'
  [brace]
  (if (= \( brace)
    1
    -1))

or one leave it untouched and convert the calling function to pass a string:

(defn calculate-floor'
  [braces floor]
  (if (empty? braces)
    floor
    (let [h (str (first braces))
          m (brace->movement h)
          f (+ floor m)]
      (calculate-floor' (rest braces) f))))

I prefer the second alternative as I have already proved to myself that the parse function works correctly when passed a string.

back to testing…

user> (calculate-floor' "(())" 0)
0
user> (calculate-floor' "()()" 0)
0

Woo hoo! And not only that, it passes all the test cases. The actual problem is a fairly long string which I don’t really want to copy and paste in to the REPL so I create a helper function to load a string from a file by adapting some code I saw in a different project written by a friend of mine (this code “just worked” and allowed me to focus on the problem that I actually wanted to solve so I didn’t question it then and I am not going to go into it now):

(ns advent.core
  (:require [clojure.java.io :as io]))

(defn read-input-string
  "returns the single string read from the file"
  [file]
  (with-open [rdr (io/reader file)]
    (first (line-seq rdr))))

I test it

user> (require '[advent.core :refer [read-input-string]])
nil
user> (def data (read-input-string "data/day-1"))
#'user/data
user> (take 10 data)
(\( \) \( \) \( \( \) \( \) \()

All looks good so I can wrap up my functions to answer day 1.

(ns advent.day-1
  (:require [advent.core :refer [read-input-string]]))

(defn day-1
  []
  (let [braces (read-input-string "data/day-1")]
    (calculate-floor' braces 0)))

user> (day-1)
java.lang.StackOverflowError: null...

Oh dear oh dear. I tried only using half of the input and I do get an answer. At first I think maybe I should divide and conquer – split the input string into a list of lists each of which is short enough to not cause a stack overflow error then combine the answers (I have heard of map reduce you see). But come on. This really isn’t a lot of input; 7000 characters. Map reduce is for big data sets and yes it might work for me here but seems like overkill and I really only want exactly the right amount of kill.

OK, maybe recursion isn’t the best way to go (NOTE: there is no problem with recursion to solve this problem and one way was suggested to me later with I go through at the end of this piece). So what other choices do I have? Well actually I start off with a list of braces I want to convert them to a list of plus or minus ones then sum them. I know that I can sum a sequence of numbers using (reduce + numbers). And actually I can create the list of numbers by using map.

(defn calculate-floor''
  [braces]
  (let [numbers (map brace->movement braces)]
    (reduce + numbers)))

Run the test cases again…

user> (calculate-floor'' "(())")
-4

Drat! I forgot the character to string conversion. But, if I supply a sequence of strings to the function it should be fine, which I can do by calling map str on a string:

user> (map str "(())")
("(" "(" ")" ")")
user> (calculate-floor'' (map str "(())"))
0

And it passes all the rest of the tests too. Good news so I am happy with this function, now to modify the calling function to pass the right thing.

(defn day-1'
  []
  (let [braces (read-input-string "data/day-1")
        list   (map str braces)]
    (calculate-floor'' list)))

That gives me an answer (280 if you are interested) which I submit and find I am correct! Woohoo! Now I am given access to the extension problem. Before I go on to that let me just give the a tidied up and complete version of all the code so far.

(ns advent.core
  (:require [clojure.java.io :as io]))

(defn read-input-string
  "returns the single string read from the file"
  [file]
  (with-open [rdr (io/reader file)]
    (first (line-seq rdr))))
(ns advent.day-1
  (:require [advent.core :refer [read-input-string]]))

(defn brace->movement
  [brace]
  (if (= "(" brace)
    1
    -1))

(defn calculate-floor
  [braces]
  (let [numbers (map brace->movement braces)]
    (reduce + numbers)))

(defn day-1
  []
  (let [braces (read-input-string "data/day-1")
        list (map str braces)]
    (calculate-floor list)))

That looks pretty short, but I don’t think that some of the intermediate assignments make much sense. The body of calculate-floor could actually be a one line without any loss of clarity:

(reduce + (map brace->movement braces))

and why introduce list in day-1? I know I need a list of strings as input so lets do the map first thing. The body would now be:

(let [braces (map str (read-input-string "data/day-1"))]
  (calculate-floor braces))

So now the cleaned up version is:

(ns advent.day-1
  (:require [advent.core :refer [read-input-string]]))

(defn brace->movement
  [brace]
  (if (= "(" brace)
    1
    -1))

(defn calculate-floor
  [braces]
  (reduce + (map brace->movement braces)))

(defn day-1
  []
  (let [braces (map str (read-input-string "data/day-1"))]
    (calculate-floor braces)))

calculate-floor looks a bit ridiculous now – is it really necessay to define a function for that one liner? I don’t think so as the only function which is not part of the language itself is brace->movement and that has already been tested so I refactor the code again to end up with:

(ns advent.day-1
  (:require [advent.core :refer [read-input-string]]))
(defn brace->movement
  [brace]
  (if (= "(" brace)
    1
    -1))

(defn day-1
  []
  (let [braces (map str (read-input-string "data/day-1"))]
    (reduce + (map brace->movement braces))))

I’m pretty happy with that, it is succinct and clear (in my estimation) and still gives the correct answer. If I had a gripe it would be that the brace->movement function is not as tightly defined as it could be; it does pass the unit tests and does its job well enough but maybe the extension introduces some new kind of brace. I decide it is worth another refactor:

(defn brace->movement
  [brace]
  (cond
    (= "(" brace) 1
    (= ")" brace) -1))

Much more satisfactory – the function does exactly what it needs to and doesn’t understand anthing else which means I should be able to pick up if there is input other than ‘(‘ or ‘)’ more quickly.

The extension

And so on to the extension. Again, these tend to follow a fairly standard pattern:

  1. Add some further constraints or stopping conditions
  2. Provide some examples/test cases for the extension
  3. Calcuate the answer using the same input set as the original problem.

For the day 1 extension I need to know the position of the brace which first makes the sum negative. First of all I notice that my refactored answer is far less testable than it used to be – I have only a single function which does everything and it accepts no input. Being able to test and refactor each method at the REPL individually whilst I was building up the solution was invaluable so I set up a stub function which will do the work for the extension and hook it into the existing functionality in a way that will allow me to test the extension:

(defn extension
  "given a sequence of plus and minus ones will return the first position for which the sum becomes negative"
  [movements])
  
(defn day-1
  []
  (let [braces (map str (read-input-string "data/day-1"))
        movements (map brace->movement braces)
        ans (reduce + movements)
        ext (extension movements)]
    (println "answer: " ans " extension: " ext)))

First of all lets create a function to convert the sequence of movements to a sequence of maps where each map contains the original movement and the index; I’ve come across map-indexed and think that is pretty much exactly what I need:

(defn add-indices
  [movements]
  (map-indexed (fn [idx mvmt] {:idx (inc idx) :mvmt mvmt}) movements))

I have now learnt my lesson and know I should test this whilst it is fresh in my mind, so here goes (though I called the argument “movements” it can be anything which makes testing nice and easy):

user> (add-indices [:a :b :c])
({:idx 1, :mvmt :a} {:idx 2, :mvmt :b} {:idx 3, :mvmt :c})

Recursion didn’t work so well for the original problem but that was because the sequence was too long and I needed to process every value in it. In this case I expressly don’t want to process every value but stop when a certain criteria has been hit so I don’t think reduce is going to help me much and I go back to recursion. A helper function extension to do the work:

(defn extension
  [movements sum]
  (let [h (first movements)
        s (+ sum (:mvmt h))]
    (if (< s 0)
      (:idx h)
      (extension (rest movements) s))))

And I can now test it:

user> (extension (add-indices [-1]) 0)
1
user> (extension (add-indices [1 -1 1 -1 -1]) 0)
5

Seems to be working on the test cases so I jump in with both feet:

user> (def braces (map str (read-input-string "data/day-1")))
#'user/braces
user> (def movements (map brace->movement braces))
#'user/movements
user> (def indexed (add-indices movements))
#'user/indexed
user> (extension indexed 0)
1797

I submit this answer and presto! I am right. But I am not satisfied; I have a solution but I don’t have a good one. What if the answer was actually 7000? I would not have been able to find that instead I would have a stack overflow error again. In fact for all I know if it had been the 3501st value that pushed Santa over the edge into the basement that would have caused a stack overflow. To prove that I have a poor solution I devise a stress test:

user> (def stress-input (flatten [(repeat 3500 "(") (repeat 3500 ")") braces]))
#'user/stress-input
user> (def movements' (map brace->movement stress-input))
#'user/movements'
user> (def indexed (add-indices movements))
#'user/indexed'
user> (extension indexed' 0)
stack overflow error

expecting 8797

Recursion seems to be the right answer, but how to do that in a scalable way? Well, I have heard of loop .. recur as a Clojurey thing so off I go to investigate that:

(defn extension
  [movements]
  (loop [indexed (add-indices movements)
         floor   0]
    (let [n          (first indexed)
          next-floor (+ floor (:mvmt n))]
      (if (< next-floor 0)
        (:idx n)
        (recur (rest indexed) next-floor)))))

user> (extension movements)
1797
user> (extension movements')
8797

And the complete solution to day 1 is now:

(ns advent.day-1
  (:require [advent.core :refer [read-input-string]]))

(defn brace->movement
  [brace]
  (cond
    (= "(" brace) 1
    (= ")" brace) -1))

(defn add-indices
  [movements]
  (map-indexed (fn [idx mvmt] {:idx (inc idx) :mvmt mvmt}) movements))

(defn extension
  [movements]
  (loop [indexed (add-indices movements)
         floor   0]
    (let [n          (first indexed)
          next-floor (+ floor (:mvmt n))]
      (if (< next-floor 0)
        (:idx n)
        (recur (rest indexed) next-floor)))))

(defn answer
  []
  (let [braces    (map str (read-input-string "data/day-1"))
        movements (map brace->movement braces)
        ans       (reduce + movements)
        ext       (extension movements)]
    (println "answer: " ans " extension: " ext)))

And that is a solution I am happy enough to leave; it has passed all the required tests and the extra stress tests I imposed. I could perhaps spend a little while coming up with better names for functions or variables but they are clear enough for me and even though I am writing this post several months after implementing the code I found the code easy enough to understand which is usually a good sign.

Conclusions

So why is it that I think advent of code is particularly good as a source of interesting problems to help people learn to code or improve how they code? (And though I have done this one in Clojure I think that many of the points are equally applicable to any language.)

In actual fact when I first solved this problem I didn’t do nearly as much refactoring as I have shown here, but when I found that each of the next few problems followed a similar pattern it encouraged me to approach them as I have shown above. I found myself writing functions which had a single responsibility and were designed to be testable and reusable. By problem three I found myself automatically assessing a solution and a function for its flexibility; for instance what if Santa had not started on the ground floor? How much code would I have to change to make that extension? How much code could I reuse from the initial solution? I also love the fact that the naïve solution (in this case recursion) will only get you so far which forced me to find out more about the language. What else have I learnt? Perhaps that you don’t always need to define a function; once I was more familiar with Clojure it seemed silly that I ever felt the need to define the original calculate-floors function and once it had been refactored to a reduce it was even more obvious to me that it didn’t need to be a named function. So why would I define a named function now? If it involved more than one line I think I would. If I wanted to test how it behaved I would. And when wouldn’t I? If it was something trivially testable at the REPL – for example (reduce + ...) or (map str ...) and I expect that the more familiar I become with the language the more likely I am to consider something trivially testable at the REPL.

Review

At Metail we like code review – so much in fact that we have double pass reviews across the board. That even goes for blog posts. We know it doesn’t guarantee perfection, but we know it does catch a lot of mistakes. Small things like typos are simply fixed by the editor/reviewer. The reviewer for this piece went beyond that by checking the implementation of the code too and has made a suggestion for an improvement. Part of the process of being open to refactoring (and code improvements) is accepting that you aren’t perfect and you didn’t get it right first time. So in this spirit rather than change the code throughout to the improved version I think I should give it as a final improvement at the end.

It is more idiomatic to use destructuring than first and rest so the extension method *could* be rewritten as:

(defn extension
  [movements]
  (loop [[head & tail] (add-indices movements)
         floor   0]
    (let [next-floor (+ floor (:mvmt head))]
      (if (< next-floor 0)
        (:idx head)
        (recur tail next-floor)))))

It was also mentioned that list was a really poor name for a variable in Clojure as it is the name of a function – but as I fixed that one myself by refactoring I wanted to leave it it.

Review 2

How fitting that we should come back to recursion. I honestly felt quite proud that after all this time I remembered my first computer science class when an enthusiastic lecturer demonstrated that tail recursion rapidly caused stack overflow issues (or whatever they are in ML) and then introduced an accumulator as an argument to the function and Lo! no more problems. So my first solution harked back to that – obviously I must have missed something because my solution had an argument I thought was an accumulator but still blew up.

Handily Clojure thinks of people like me and saves them from themselves by providing a way to do recursion without having to remember any computer science (or using loop). So I could have rewritten my original calculation as:

(defn calculate-floor
  [braces floor]
  (if (empty? braces)
    floor
    (let [f (first braces)
          r (rest braces)
          m (brace->movement f)]
      (recur r (+ floor m)))))

However the review and I both agreed that the version using map which I found myself was better than this implementation. They also suggested further changes to the brace->movement function – using case rather than cond and went on to say that if this were a true code review reduce and reduced would be brought up as interesting corners of the language to explore. It was then that I backed away.

Thanks to the reviewers for the opportunity to learn 🙂

Now this post is written I can get back to the next problem and I very much hope that there will be another set come advent 2016.