This week, I was introduced to Hopleaf, a craft beer bar in Chicago with 60 rotating taps. One of my favorite bars in Tucson, 1702, was similar. But every time I visited, I would face a dilemma: I usually only visit twice a month, and drink 3±1 beers each time. As a reformed economist, I insist on optimizing every decision I make, and with each beer I order, I face a trade-off between tasting a new beer of uncertain quality or choosing my favorite so far. If I want to maximize expected utility for each month’s visits, what is my best strategy? How many beers should I spend exploring for better ones, and how many should I spend enjoying my favorite?
Richard Feynman posed this problem in the context of finding the best dish at a new restaurant. (But beers are way more interesting). The solution makes sense (and come on, it’s Richard Feynman), but I wanted to check his work with a simulation.
You can find the results (and the rest of this post) in this iPython notebook. There’s much more there, including an interesting result!
Martin Fowler identifies long methods as another common code smell, fixable by breaking one long method into several smaller ones and composing them together. Since Clean Code emphasized writing short, meaningful methods, I had to look a bit to find one. But I’m not very happy with the board coordinate constructor:
1234567891011
publicUniversalBoardCoordinate(StringlocationPhrase)throwsInvalidCoordinateException{StringnoParens=locationPhrase.replace('(',' ').replace(')',' ');String[]coordinates=noParens.split(",");if(coordinates.length!=2){thrownewInvalidCoordinateException("That's not a valid board location.");}row=Integer.parseInt(coordinates[0].trim());column=Integer.parseInt(coordinates[1].trim());}
This is an easy refactor: think about what each line of code does, group the related ones in their own methods, and replace them. In fact, I’d already separated each group with a line break. The first two trim the input string and split it in two:
privatevoidcheckValidity(String[]coordinates)throwsInvalidCoordinateException{if(coordinates.length!=2){thrownewInvalidCoordinateException("That's not a valid board location.");}}
Before finishing up the first version of my terminal view game, I moved strings like the welcome message and player prompts to a properties file. This requires reading and storing the strings before using them in the view, but has two big advantages. First, tests no longer break when I edit a string (at least, as long as my tests are using the same properties). Second, if I wanted to translate my program into another language, it’s as easy as swapping out the properties file.
Unfortunately, loading strings got ugly fast. Here’s an example from the GameController class:
It gets worse. When I started writing my Swing view, I needed to ignore certain strings, like those that prompt for keyboard input. A good idea: avoid creating a brand new SwingController class, but somehow filter out unneeded strings. A bad idea: do it by checking and ignoring certain strings from the controller. This is logic that really shouldn’t be in the view, implemented in a very fragile way. In fact, it undoes all the abstraction of the properties file—as soon as a string changes, displayMessage() will probably break. Plus, there’s plenty of duplicated code.
This is similar, though not identical to Fowler’s “Refused bequest,” where a subclass inherits lots of methods and then ignores them. Here’ I’m loading lots of strings, then ignoring them.
Refused bequest is fixed with a refactor called “Replace inheritance with delegation:” put the superclass in a field on the old subclass, remove the inheritance, and simply delegate to the superclass methods when needed. Here, I haven’t even shared code through inheritance, but by good old copy-and-paste (so it’s also part of the duplicated code stink parade).
The cleanup strategy here is similar too: create a separate class to handle loading properties, and use it in place of the duplicated methods. I’ll start by creating a StringLoader class that includes the duplicated fields and methods:
The repeated calls to viewStrings.getProperty() are still pretty ugly and very difficult to read. One solution is to extract a load() method, then iterate over an array of the property names:
This requires trading off a little clarity, but on balance it’s much cleaner. Best of all, now I can use properties as they were intended. Preventing my view from displaying certain strings just requires creating a new properties file and removing the content from the unneeded messages—another win for decoupling and abstraction.
I’ve spent the last couple weeks working through Martin Fowler’sRefactoring, which includes a taxonomy of useful solutions to common code smells. It’s an excellent book, but I found it hard to recognize and remember some of the patterns without digging into my own code. Over the next few posts, I’ll describe some of the smells and refactors I found in my Tic Tac Toe game (as much to commit them to my own memory as to share them with others). First up, the most common code smell of all: duplication.
Despite using a superclass to store static methods and data
used by all my tests, I wasn’t using inheritance to create universal setup and teardown methods. Although many of my tests used a similar pattern to capture output, none were quite alike, and all of them were pretty messy. Here’s an example:
These tests use both a plain PrintStream and my custom OutputRecorder, though they don’t really need to. Some setup is done in the fields, and some during setUp(), though there’s no clear reason why. And the same lines setting up and tearing down the recorder are repeated across lots of tests. The smell here is duplicated code, which Fowler calls “number one in the stink parade.” To solve it, I’ll start by extracting a method, and then extracting a superclass.
First, I’ll create an empty TicTacToeTest class with empty setup and teardown methods.
The last line of the second test was meant as a mini-teardown, to reset stdout before the next test. I wrote it before I understood JUnit execution, in which each test runs in its own environment. It survived around 80 commits, but it doesn’t actually do anything, so I can safely delete it. (I know because none of the tests fail afterwards). In fact, this whole refactor should be possible while keeping the tests green.
After searching for usages of the old pattern and replacing them with the new methods, there’s just one thing left: pull up the new methods to a Test superclass. Here’s the result:
This looks like a simple refactor, but my tests are now much cleaner. More important, if I need to make a change to the OutputRecorder class, it will propagate through all tests.
One of last week’s projects was setting up Travis
CI, a hosted continuous
integration
service that’s free for open source projects. Even if you haven’t
heard about Travis (as I hadn’t), you’ve probably already seen the
service on Github—it’s responsible for the green and red build status
icons at the top of many project READMEs.
After every push to a remote repo, Travis builds your project, runs
the test suite, and sends off a notification with the results. Once
it’s configured, it’s a great way to ensure that even if your project is
unfinished, the published code will work correctly for anyone who
pulls the repo.
The official Travis docs are close to
comprehensive, but I ran into a few catches getting everything set up (and have an inbox full of
failed build notifications to prove it). Hopefully these solutions
will rescue someone else from a frustrating afternoon.
Resolving Ivy dependencies
My Tic Tac Toe project uses Ivy to
manage build dependencies, and my ant build script includes a standard
snippet that downloads Ivy if it’s not available locally:
<propertyname="ivy.install.version"value="2.1.0-rc2"/><conditionproperty="ivy.home"value="${env.IVY_HOME}"><issetproperty="env.IVY_HOME"/></condition><propertyname="ivy.home"value="${user.home}/.ant"/><propertyname="ivy.jar.dir"value="${ivy.home}/lib"/><propertyname="ivy.jar.file"value="${ivy.jar.dir}/ivy.jar"/><targetname="download-ivy"unless="offline"><mkdirdir="${ivy.jar.dir}"/><!-- download Ivy from web site so that it can be used even without -- -- any special installation --><getsrc="http://repo2.maven.org/maven2/org/apache/ivy/ivy/${ivy.install.version}/ivy-${ivy.install.version}.jar"dest="${ivy.jar.file}"usetimestamp="true"/></target><targetname="init-ivy"depends="download-ivy"><!-- try to load ivy here from ivy home, in case the user has not -- -- already dropped it into ant's lib dir (note that the latter copy -- -- will always take precedence). We will not fail as long as local -- -- lib dir exists (it may be empty) and ivy is in at least one of -- -- ant's lib dir or the local lib dir. --><pathid="ivy.lib.path"><filesetdir="${ivy.jar.dir}"includes="*.jar"/></path><taskdefresource="org/apache/ivy/ant/antlib.xml"uri="antlib:org.apache.ivy.ant"classpathref="ivy.lib.path"/></target>
If your build and test targets are set to depend on the init-ivy
task, the script will ensure that Ivy resolves itself and the
project’s other dependencies correctly. Telling Travis how to build
and test a project is as simple as writing a few lines of YAML. For
example:
This tells Travis the project language (which comes with its own
default settings),
points to ant resolve as the command necessary to resolve
dependencies, and describes the target Java versions to test against.
But my builds failed with the following mysterious error:
$ ant resolve
Buildfile: /home/travis/build/ecmendenhall/Java-TTT/build.xml
[mkdir] Created dir: /home/travis/build/ecmendenhall/Java-TTT/lib
check-ivy:
[echo] Checking for Ivy .jar in local directories.
bootstrap-ivy:
[echo] Bootstrapping Ivy installation.
[mkdir] Created dir: /home/travis/.ant/lib
[get] Getting: http://search.maven.org/remotecontent?filepath=org/apache/ivy/ivy/2.3.0/ivy-2.3.0.jar
[get] To: /home/travis/.ant/lib/ivy.jar
resolve:
[echo] Resolving project dependencies.
BUILD FAILED
/home/travis/build/ecmendenhall/Java-TTT/build.xml:52: Problem: failed
to create task or type antlib:org.apache.ivy.ant:retrieve
Cause: The name is undefined.
Action: Check the spelling.
Action: Check that any custom tasks/types have been declared.
Action: Check that any <presetdef>/<macrodef> declarations have taken
place.
No types or tasks have been defined in this namespace yet
This appears to be an antlib declaration.
Action: Check that the implementing library exists in one of:
-/usr/share/ant/lib
-/home/travis/.ant/lib
-a directory added on the command line with the -lib argument
Even though ant was correctly downloading an ivy jarfile, the build
script failed to find it. The answers to my Stack Overflow question
explained that this was related to the order in which jars are
loaded to the Java classpath. The easiest solution is to run ant
twice, once to download ivy, and again to build and test the
project.
Once the source of this error was cleared up, the solution was simple.
Adding a before_install script to my Travis build ensured that ivy
would be downloaded before trying to build the project. This meant
adding just one line to my .travis.yml:
There are a number of other options to run scripts at different times
in the Travis lifecycle. Check out the rest of the docs
here.
Headless testing with xfvb
My Travis builds worked nicely until I started adding a Swing view to
my Tic Tac Toe project. Although the tests would run and pass locally,
Travis threw a java.awt.HeadlessException as soon as it tried to run
the test suite. The Travis docs explain using a tool called xvfb (X
Virtual Framebuffer) to simulate a windowing system and run headless
tests, but warn that “you need to tell your testing tool process”
exactly how to use it.
Exactly how to do this wasn’t clear, but after fiddling with JVM startup arguments and
trying to shell out from inside ant, I discovered that the solution
was dead simple: just add the recommended arguments to .travis.yml,
and ant and JUnit will take care of the rest. Here’s my final
.travis.yml, including the solutions to both problems.
“It is a revelation to compare Menard’s Don Quixote with Cervantes’.
The latter, for example, wrote (part one, chapter nine):
‘…truth, whose mother is history, rival of time, depository of
deeds, witness of the past, exemplar and adviser to the present, and
the future’s counselor.’ Written in the seventeenth century, written by
the ‘lay genius’ Cervantes, this enumeration is a mere rhetorical
praise of history. Menard, on the other hand, writes:
‘…truth, whose mother is history, rival of time, depository of
deeds, witness of the past, exemplar and adviser to the present, and
the future’s counselor.’”
Macros are hard,
but one of the most helpful exercises I’ve found in my limited macro
writing experience is practicing by recreating some of the core
macros I already know and love, like or,
when-let, and ->.
Using speclj greatly simplifies
this exercise, and writing macro specs often test my understanding
better than writing the implementations themselves. Here’s an example spec for the threading macro:
12345678910111213
(describe"-> macro"(it"should expand into the code below"(let [macro-form'(->-macro1(+ 2)(* 3))](should='(macro-challenges.core/->-macro(macro-challenges.core/->-macro1(+ 2))(* 3))(macroexpand-1 macro-form))(should='(* (macro-challenges.core/->-macro1(+ 2))3)(macroexpand macro-form))(should='(* (+ 12)3)(macroexpand-allmacro-form)))))
The functions macroexpand, macroexpand-1, and macroexpand-all
come in very handy. Macroexpand-1 returns the “first” expansion of a
macro form (macros within macros won’t be expanded). Macroexpand
calls macroexpand-1 until the expansion is no longer a macro form.
In the above example, the first expansion of ->-macro, my threading
macro replacement, returns a form that starts with another call to
->-macro. (I was surprised to find out that this is how the
threading macro works under the hood). Macroexpand expands into a
list
until the first item is *, which is not a macro.
When it’s macros all the way down, macroexpand-all (technically
clojure.walk/macroexpand-all) recursively expands all macros in a
given form, resulting in the much simpler expression (* (+ 1 2) 3)
in the example above. These functions are all hugely helpful for
writing macros and their associated tests.
Here’s my recreation of ->, which passed the spec:
And bears a pretty strong resemblance to the original source:
1234567891011
(defmacro ->"Threads the expr through the forms. Inserts x as the second item in the first form, making a list of it if it is not a list already. If there are more forms, inserts the first form as the second item in second form, etc."{:added"1.0"}([x]x)([xform](if (seq? form)(with-meta `(~(first form)~x~@(next form))(meta form))(list formx)))([xform&more]`(-> (-> ~x~form)~@more)))
Some macros are pretty easy to reconstruct (but check their
documentation
to make sure you really understand how they handle different
arguments). If you’re well and truly stuck, it’s always possible to check out the
original
code for inspiration.
The specs and solutions I’ve written so far (mostly low-hanging fruit) are all
available on my
Github, if you’d
like to have a hand at reconstructing Clojure yourself.
Clojure may be a new language, but Lisp has a long history.
Translating Scheme and Common Lisp classics into Clojure is always an
interesting exercise, and often illuminates the differences and
comparative advantages of various Lisp-y languages. (For more
Lisp-to-Clojure resources see this
list.
Or, if you’d like to try porting some Scheme, consider helping
translate SICP).
This weekend, I spent some time with Paul Graham’s classic On
Lisp.
In Chapter 6, Graham shows how to use
closures
to model nodes in a network, representing a 20 questions game as a self-traversing binary
tree. Here’s his original code, and my best attempts at Clojure translations.
The most obvious model for a set of connected nodes is a nested data
structure, like a map of maps. In Common Lisp, Graham uses a mutable
hashmap of structured
types,
each pointing to their neighbors in the network:
A simple map seems like a sufficient replacement in Clojure. A single
node looks like this:
1
{:people{:contents"Is the person a man?", :yes:male, :no:female}}
And the full tree looks like the following. Each node’s :yes or :no keyword
points to the next node in the tree:
123456
{:penny{:contents"Abraham Lincoln.", :yesnil, :nonil},
:coin{:contents"Is the coin a penny?", :yes:penny, :no:other-coin},
:USA{:contents"Is he on a coin?", :yes:coin, :no:no-coin},
:dead{:contents"Was he from the USA?", :yes:USA, :no:elsewhere},
:male{:contents"Is he living?", :yes:live, :no:dead},
:people{:contents"Is the person a man?", :yes:male, :no:female}}
Here’s my first draft for defining nodes in Clojure. Since the Common
Lisp version used a mutable variable, I used a Clojure atom to store
network state:
123456789101112
(def nodes(atom{}))(defn defnode[name contents&[yesno]](swap!nodesassoc name {:contentscontents:yesyes:nono}))(defn make-nodes[](defnode:people"Is the person a man?":male:female)(defnode:male"Is he living?":live:dead)(defnode:dead"Was he from the USA?":USA:elsewhere)(defnode:USA"Is he on a coin?":coin:no-coin)(defnode:coin"Is the coin a penny?":penny:other-coin)(defnode:penny"Abraham Lincoln."))
Traversing the network is simple: get a node, print the associated
question, prompt for input, get the next node, and repeat. Here’s the original Common Lisp:
Of course, there’s no reason to bother swapping and dereferencing an
atom as long as the tree won’t need to change at runtime. This
immutable version works just as well:
1234567891011121314151617181920212223242526
(defn defnode[nodesname contents&[yesno]](assoc nodesname {:contentscontents:yesyes:nono}))(defn make-nodes[](-> {}(defnode:people"Is the person a man?":male:female)(defnode:male"Is he living?":live:dead)(defnode:dead"Was he from the USA?":USA:elsewhere)(defnode:USA"Is he on a coin?":coin:no-coin)(defnode:coin"Is the coin a penny?":penny:other-coin)(defnode:penny"Abraham Lincoln.")))(def nodes(make-nodes))(defn run-node[nodesname](let [node (nodesname)contents(node :contents)yes(node :yes)no(node :no)](if yes(do(println contents)(if (= "yes"(read-line))(run-nodenodesyes)(run-nodenodesno)))(println contents))))
Using a closure rolls the data structure and traversal code into one,
by associating the yes and no fields with anonymous functions that
handle the same logic as run-node. Here’s Graham’s CL version:
And here’s mine in Clojure. The double-parens around (@nodes yes)
and (@nodes no) call the anonymous function, instead of just
returning it.
1234567891011121314151617181920
(def nodes(atom{}))(defn defclosure[name contents&[yesno]](swap!nodesassoc name(if yes(fn [](println contents)(if (= "yes"(read-line))((@nodesyes))((@nodesno))))(fn [](println contents)))))(defn make-nodes[](defclosure:people"Is the person a man?":male:female)(defclosure:male"Is he living?":live:dead)(defclosure:dead"Was he from the USA?":USA:elsewhere)(defclosure:USA"Is he on a coin?":coin:no-coin)(defclosure:coin"Is the coin a penny?":penny:other-coin)(defclosure:penny"Abraham Lincoln."))
Now, traversing the tree is as simple as calling:
1
((nodes:people))
And watching the tree traverse itself. You can find my code from this
post here. For more on closures in Lisp,
check out the rest of Chapter 6.
One more recipe for the TDD cookbook before I move on to more
interesting things. You might have noticed a new GameOverException
and a try-catch block in the final example of my last post.
Before adding a game over exception, methods that checked for final
game states directly called System.exit(). Testing this proved
difficult, and I ended up importing a third-party library of extra
JUnit rules. This week, I refactored them to throw a custom exception:
This is not only a better practice,
but also provides a much better way to test methods that might detect
a completed game—just add a try/catch block to the tests!
Exceptions have come in handy elsewhere in my tests, too. Once the
controller starts a game, there’s nothing to break the back-and-forth
game loop but a win, draw, or error. Figuring out how to test game
states without getting stuck in an infinite loop or loading up entire
games was a challenge. “If only there
were some special syntax for breaking normal control flow in special
situations,” I wondered to myself more times than I’d like to admit.
Well, duh—use exceptions! My mock view objects throw a NoSuchElementException when their input
queue runs empty. Catching this exception breaks the normal game flow
and allows me to access game state as soon as the fake input I’m
interested in has been sent to the controller. Here’s an example:
GameControllerTest.java
12345678910111213141516171819202122
@TestpublicvoidcontrollerShouldPassErrorMessageToViewOnInvalidInput()throwsGameOverException{view.enqueueInput("invalid phrase");controller.newGame();System.setOut(outputRecorder);try{controller.playRound();}catch(NoSuchElementExceptione){outputRecorder.discardFirstNStrings(1);Stringoutput=outputRecorder.popFirstOutput();assertEquals("That's not a valid board location.",output);}System.setOut(stdout);view.clearInput();}
Normally, Controller.playRound() will continue querying players for
moves until the game ends. But once this test catches the empty queue
exception, it tests against the expected output, which should show an
error message. Exceptions have proved extremely handy so far—as long as I remember
that they’re in my control flow toolbox, too.
Redirecting stdout to a new PrintStream is an easy way
to test simple console output in Java. But as my Tic-Tac-Toe game has
grown more complex, the tests I wrote using this pattern have started
to stink. There’s a lot of duplicated code (create the stream,
redirect, tear down with each test), each test uses a hand-constructed
string filled with finicky newline characters, and tests are prone to
break when unrelated view components change the way they print to the
screen. Inspired by my input
queue,
I created an OutputRecorder class that extends PrintStream and
captures output string by string for later playback:
@TestpublicvoidcontrollerShouldPassErrorMessageToViewOnInvalidMove()throwsGameOverException{view.enqueueInput("middle center");view.enqueueInput("middle center");controller.newGame();System.setOut(outputRecorder);try{controller.playRound();}catch(NoSuchElementExceptione){outputRecorder.discardFirstNStrings(4);Stringoutput=outputRecorder.popFirstOutput();assertEquals("Square is already full.",output);}System.setOut(stdout);view.clearInput();}
The utility might not be immediately obvious, but capturing output
string by string has already put an end to tracking down small
differences between expected and actual output that come from an extra
space or misplaced newline.
Over the course of my Tic Tac Toe project, I’ve needed to test against
user input several times. As a newcomer to mock object
patterns,
coming up with good solutions to these testing dilemmas has been one
of my biggest challenges.
There are plenty of heavy-duty
tools
for mocks and fakes in Java, but I’d like to stick with my own
solutions as long as possible, since writing them myself has been
enlightening.
Here’s a solution I came up with today to simulate full Tic Tac Toe
games with two human players: a mock View object that returns fake input
from a queue. By pushing mock input onto the queue during test setup,
I can configure games in advance and replay them later. Here’s the
very simple mock View object:
And here’s an example test that plays through an entire game and exits
when Player 2 wins (comments added for some context):
GameControllerTest.java
12345678910111213141516171819202122
@TestpublicvoidgameShouldEndOnWin(){exit.expectSystemExit();controller.newGame();// Select two human playersview.enqueueInput("h");view.enqueueInput("h");controller.setUp();view.enqueueInput("middle center");// Player 1's first moveview.enqueueInput("top left");// Player 2's first moveview.enqueueInput("top right");// Player 1 goes for the diagonal...view.enqueueInput("middle left");// Player 2 goes for the column...view.enqueueInput("lower right");// Player 1 chokes!view.enqueueInput("lower left");// Player 2 wins! What an upset! controller.startGame();}
I’m sure I’ll discover the shortcomings of this approach sooner or
later, but for now it’s a pretty good way to test events inside
the main game loop.