Wednesday, December 23, 2015

Looking for problematic logging with JUnit

Stefan Birkner's System Rules is one of my favorite JUnit extension libraries. I commonly use it to verify System.out and System.err, for example validating audit trail logging.

Growing tired of the same boilerplate, I rolled some simple rules into an aggregated JUnit @Rule, called NiceLoggingRule. It enforces:

  • No logging to System.err
  • No WARN or ERROR logging to System.out

A more sophisticated version would let the user decide on more than "log level" as to what is an acceptable log line, but it gives a good demonstration of writing complex JUnit rules:

public final class NiceLoggingRule
        implements TestRule {
    private static final Pattern NEWLINE = compile("\n");

    private final SystemOutRule sout = new SystemOutRule().
            enableLog().
            muteForSuccessfulTests();
    private final SystemErrRule serr = new SystemErrRule().
            enableLog();

    private final Pattern logLinePattern;
    private final Predicate<String> problematic;
    private final RuleChain delegate;

    public NiceLoggingRule(final String logLinePattern,
            final Predicate<String> problematic) {
        this.logLinePattern = compile(logLinePattern);
        this.problematic = problematic;
        delegate = outerRule(NiceLoggingStatement::new).
                around(sout).
                around(serr);
    }

    @Override
    public Statement apply(final Statement base,
            final Description description) {
        return delegate.apply(base, description);
    }

    private final class NiceLoggingStatement
            extends Statement {
        private final Statement base;
        private final Description description;

        private NiceLoggingStatement(final Statement base,
                final Description description) {
            this.base = base;
            this.description = description;
        }

        @Override
        public void evaluate()
                throws Throwable {
            base.evaluate();
            checkSystemErr(description);
            checkSystemOut(description);
        }

        private void checkSystemErr(final Description description) {
            final String cleanSerr = serr.getLogWithNormalizedLineSeparator();
            final List<String> errors = NEWLINE.splitAsStream(cleanSerr).
                    collect(toList());
            if (!errors.isEmpty())
                fail("Output to System.err from " + description + ":\n"
                        + cleanSerr);
        }

        private void checkSystemOut(final Description description) {
            final String cleanSout = sout.getLogWithNormalizedLineSeparator();
            final List<LogLine> problems = NEWLINE.splitAsStream(cleanSout).
                    map(LogLine::new).
                    filter(LogLine::problematic).
                    collect(toList());
            if (!problems.isEmpty())
                fail(problems.stream().
                        map(Object::toString).
                        collect(joining("",
                                "Problems to System.out from " + description
                                        + ":\n", "")));
        }
    }

    private final class LogLine {
        @Nonnull
        private final String line;
        @Nonnull
        private final String level;

        private LogLine(@Nonnull final String line) {
            final Matcher match = logLinePattern.matcher(line);
            if (!match.find()) // Not match! Ignore trailing CR?NL
                fail(format(
                        "Log line does not match expected pattern (%s): %s",
                        logLinePattern.pattern(), line));
            this.line = line;
            level = match.group("level");
        }

        public boolean problematic() {
            return problematic.test(level);
        }

        @Override
        public String toString() {
            return line;
        }
    }
}

For example, using it with Spring Boot's default log pattern one might write a factory helper:

public final class SpringDefaultNiceLoggingRule {
    private static final String logLevels = Stream.of(LogLevel.values()).
            filter(level -> OFF != level).
            map(Enum::name).
            collect(joining("|"));

    public static NiceLoggingRule springDefaultNiceLoggingRule() {
        return new NiceLoggingRule(
                "^(?<timestamp>\\d{4,4}-\\d{2,2}-\\d{2,2} \\d{2,2}:\\d{2,2}:\\d{2,2}\\.\\d{3,3}) +(?<level>"
                        + logLevels + ") +",
                SpringDefaultNiceLoggingRule::problematic);
    }

    private static boolean problematic(final String level) {
        return 0 > INFO.compareTo(LogLevel.valueOf(level));
    }
}

Then a simple Spring Boot unit test becomes:

@RunWith(SpringJUnit4ClassRunner.class)
@SpringApplicationConfiguration(classes = MockServletContext.class)
public final class RootControllerTest {
    @Rule
    public final NiceLoggingRule niceLogging = springDefaultNiceLoggingRule();

    private MockMvc mvc;

    @Before
    public void setUp()
            throws Exception {
        mvc = standaloneSetup(new RootController()).build();
    }

    @Test
    public void shouldGetRoot()
            throws Exception {
        mvc.perform(get("/").
                accept(APPLICATION_JSON_UTF8)).
                andExpect(status().isOk()).
                andExpect(jsonPath("$.message", equalTo("Hello, world!")));
    }
}

Monday, December 21, 2015

RESTful helper script

I find this script useful working on modern RESTful services. It shows both the headers and the formatted JSON response body. The idea is that most times you provide a URL and want to see the full response. If you need extra flags for curl just add them (e.g., user/password). If you want to customize jq—say, filter for just a particular piece of the response—use a double-dash ("--") to separate curl and jq arguments:

An example with Spring Boot (plus some custom actuator endpoints). Note "jq" colorizes the output on the command line (below is plain text):

$ ~/bin/jurlq http://localhost:8081/remote-hello/health
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Strict-Transport-Security: max-age=31536000 ; includeSubDomains
X-Application-Context: remote-hello:8081
Content-Type: application/json;charset=UTF-8
Transfer-Encoding: chunked
Date: Mon, 21 Dec 2015 13:28:07 GMT

{
  "status": "UP",
  "cpu": {
    "status": "UP",
    "processors": 8,
    "system-loadavg": -1,
    "process-cpu-load": 0.22963892677420872,
    "process-cpu-time": "PT42.71875S",
    "system-cpu-load": -1
  },
  "file": {
    "status": "UP",
    "usable-disk": 55486464000,
    "total-disk": 299694551040
  },
  "java": {
    "status": "UP",
    "start-time": "2015-12-21T07:27:06.970-06:00",
    "uptime-beats": 0,
    "vm-name": "Java HotSpot(TM) 64-Bit Server VM",
    "vm-vendor": "Oracle Corporation",
    "vm-version": "25.66-b17"
  },
  "memory": {
    "status": "UP",
    "committed-virtual-memory": 991350784,
    "free-physical-memory": 1952854016,
    "free-swap-space": 1203003392,
    "total-physical-memory": 8540618752,
    "total-swap-space": 10354229248
  },
  "os": {
    "status": "UP",
    "arch": "amd64",
    "name": "Windows 10",
    "version": "10.0"
  },
  "threads": {
    "status": "UP",
    "count": 22,
    "daemon-count": 20,
    "peak-count": 22,
    "started-count": 26
  },
  "diskSpace": {
    "status": "UP",
    "free": 55486464000,
    "threshold": 10485760
  },
  "configServer": {
    "status": "UNKNOWN",
    "error": "no property sources located"
  },
  "hystrix": {
    "status": "UP"
  }
}

UPDATE: Tried Github Gist for the source, but it does not show in my blog feed reader. Here's the script:

#!/bin/bash

curl_args=()
for arg
do
    case "$arg" in
    -- ) shift ; break ;;
    * ) curl_args=("${curl_args[@]}" "$arg") ; shift ;;
    esac
done

jq_args=("${@-.}")

curl -s -D - "${curl_args[@]}" | tr -d '\r' | {
    while read line
    do
        case "$line" in
        '' ) echo ; break ;;
        * ) echo "$line" ;;
        esac
    done

    exec jq "${jq_args[@]}"
}

Saturday, December 19, 2015

Spring REST showcase

I've noodled for some time now at mini-projects showcasing Spring REST with as many bells and whistles as I could pack in before it broke. I've never reached a satisfactory conclusion, which is more a testament to my mercurial temperament than to Spring. My ideal project would include:

  • No XML, if avoidable (Lombok, I'm looking at you)
  • No properties files (thank you, Spring YAML support)
  • Annotations and code generation (thank you, Spring Boot and Lombok)
  • Good documentation (Swagger and RAML, why do you need to be so tricky?)
  • Complete REST adherence (HATEOAS, you are still an ugly child, sorry to say that)
  • Modern Java
  • No external container (Spring Boot to the rescue!)
  • Many example integration points (and this is why I stay with you Spring)
  • Maven build (sorry Gradle, I gave up makefiles so I would never again debug my build)
  • Production support (Spring actuator: genius; see OSI)
  • Cloud support
  • Full CD pipeline (Boxfuse, you may save me yet; Github and Travis, you're still the best)

Essentially I want to implement a showcase REST microservice adhering to Larry Wall's three great virtues of a programmer: Laziness, Impatience and Hubris.

Well, there's always yet another Github repo.

Updates I'll keep updating this post as I find new things to desire in a showcase project.

Friday, November 06, 2015

Wrong, but technically right

tldr;Immutable objects should have all-final fields.

I'm reading one of those "interview questions" posts and this jumped out at me:

Question 3: Does all property of Immutable Object needs to be final? (answer) Not necessary, as stated above you can achieve same functionality by making member as non-final but private and not modifying them except in constructor. Don't provide setter method for them and if it is a mutable object, then don't ever leak any reference for that member. Remember making a reference variable final, only ensures that it will not be reassigned a different value, but you can still change individual properties of object, pointed by that reference variable. This is one of the key point, Interviewer like to hear from candidates.

This is unfortunate. Although technically true, in my experience I've found the non-final case to come up <1% of the time. It's terribly misleading for less experienced developers to see an answer like this without having the context that goes with it.

To understand why I'm trepidatious, you can't go wrong with the C2 wiki on value objects. The right pattern for immutable objects in Java goes something like:

@EqualsAndHashCode
@RequiredArgsConstructor(staticName = "valueOf")
@ToString
public final class HiMomImImmutable {
    public final int i;
    public final String s;
}

Here I use Lombok annotations as shorthand for the equivalent Java code. If you're not familiar with Lombok, please become so—it's both a time-saver and a bug-killer in your code.

Lombok has a @Value annotation for these. However I disagree that with their choice that fields should be private and accessed with getters. To me this makes sense when consuming frameworks requiring bean-like getters, but not otherwise.

Back to the post, the author's answer is unclear: he mentions modifying final fields in the constructor. But of course, any field marked final may be assigned in the constructor—must be so assigned—if not initialized in declaration. Then if you have complex logic to initialize (itself a code smell), try this:

public SomeClass(/* args */) {
    this.someField = aStaticMethod(/* some other args */);
}

This encapsulates the initialization logic in a static method as the final field may be initialized only once, and not referred to before initialization.

Monday, October 26, 2015

Approximating tuples in Java

What is a tuple? It's an ordered collection of individually typed elements. Wikipedia has a complicated explanation that comes down to the same thing in the context of ordinary programming. The Python implementation of tuples is a good example.

There are many related concepts, such as:

Arrays and lists
Ordered but all elements are the same type
Ordered dictionaries (maps)
Ordered but all values are of the same type, and they are named
Aggregates (structs, unions)
Individually typed but named and unordered

The closest matches to tuples in current Java are "structs" (more below) and arrays or lists, each with drawbacks. For most use of tuples, arrays and lists are non-starters: elements are all of the same nominal type. There is a lot of thought around how to do this best in Java.

What do I mean by saying Java has "structs" ala "C"? The vast bulk of Java code hides fields away with private, but exposes them with getter methods (and setters when non-final). This is the "Java Bean" anti-pattern. An alternative is to simply expose fields directly:

public final class CartesianPoint {
    public final int x;
    public final int y;

    public CartesianPoint(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

This is almost a tuple. However:

  • CartesianPoint extends java.lang.Object but should not be an object in the OOP sense
  • Elements are accessed by name, e.g., point.x, rather than position
  • Elements are unordered—you cannot write a general function to process the first and second elements of the tuple without knowing 1) the struct type and 2) the names of the fields

But for many use cases this can be close enough, for example as a return type to simulate multiple value return. Hopefully Java 10 brings value types which resolves the java.lang.Object issue. This may even bring true tuples!

This example can be improved with Lombok:

@EqualsAndHashCode
@RequiredArgsConstructor(staticName = "valueOf")
@ToString(includeFieldNames = false)
public final class CartesianPoint {
    public final int x;
    public final int y;
}

Calling code would look like:

public CartesianPoint locate() {
    int x = computeX();
    int y = computeY();

    return CartesianPoint.valueOf(x, y);
}

Notice the visual similarity of the local variables x and y to the corresponding fields in CartesianPoint. Users of CartesianPoint would see:

public static void main(final String... args) {
    Boat boat = Boat.rowBoat();
    CartesianPoint point = boat.locate();

    out.printf("%s -> x is %d, y is %d%n", point, point.x, point.y);
}

$ ./float-boat 1 2
CartesianPoint(1, 2) -> x is 1, y is 2

If you want to go hog wild, the ordered and unnamed qualities of tuples can be simulated though not without significant noise:

Function<CartesianPoint, Integer> first = p -> p.x;
Function<CartesianPoint, Integer> second = p -> p.y;

out.printf("first is %d, second is %d%n",
        first.apply(point),
        second.apply(point));

Update

Streaming functionally:

Function<CartesianPoint, Integer> first = p -> p.x;
Function<CartesianPoint, Integer> second = p -> p.y;
Stream.of(first, second).
    map(f -> f.apply(point)).
    forEach(out::println);

Sunday, October 25, 2015

What's wrong with Java 8 series by Pierre-Yves Saumont

Pierre-Yves Saumont wrote a series of articles for DZone. I feel remiss for having missed them. For example, What's Wrong in Java 8, Part IV: Monads explores java.util.Optional. Do not be misled by the post titles—yes, he criticizes Java 8 for what it could have been—, but he covers functional thinking in Java with depth, skill and panache.

The full list of "What's Wrong with Java 8" articles:

  1. Currying vs Closures
  2. Functions & Primitives
  3. Streams and Parallel Streams
  4. Monads
  5. Tuples
  6. Strictness
  7. Streams again

And all his DZone articles.

Wednesday, October 21, 2015

Tracking Java 8 stream count

I had a coding problem to fail when a Java 8 stream was empty. One way is illustrated below in example1. Another approach was interesting, shown in example2

public final class Streamy {
    private static <T, E extends RuntimeException> void example1(
            final Stream<T> items,
            final Consumer<? super T> process,
            final Supplier<E> thrown)
            throws E {
        if (0 == items.
                peek(process).
                count())
            throw thrown.get();
    }

    private static <T, E extends RuntimeException> void example2(
            final Stream<T> items,
            final Consumer<? super T> process,
            final Supplier<E> thrown)
            throws E {
        final AtomicBoolean foundSome = new AtomicBoolean();
        try (final Stream<T> stream = items) {
            stream.
                    onClose(() -> {
                        if (!foundSome.get())
                            throw thrown.get();
                    }).
                    peek(__ -> foundSome.set(true)).
                    forEach(process);
        }
    }

    public static void main(final String... args) {
        example1(Stream.of(), out::println, RuntimeException::new);
        example2(Stream.of(), out::println, RuntimeException::new);
    }
}

Comparing them I find:

  • Using count() is shorter and more clear
  • Using onClose() is more expressive

I found it odd to use peek() in example1 to execute the real purpose of the code, and was happy to discover onClose() though disappointed to need try-with-resources for it to run.

It was unfortunate that the more expressive approach (peek() for side effect, forEach() for processing, onClose for post-processing check) was also harder to understand.

Sunday, October 18, 2015

Downloading sources and javadocs automatically

Thanks to Ted Wise, I taught my "modern java" build to automatically download sources and javadocs. Using maven:

<plugin>
    <artifactId>maven-dependency-plugin</artifactId>
    <version>${maven-dependency-plugin.version}</version>
    <executions>
        <execution>
            <id>download-sources</id>
            <phase>generate-sources</phase>
            <goals>
                <goal>sources</goal>
            </goals>
        </execution>
        <execution>
            <id>download-javadocs</id>
            <phase>generate-sources</phase>
            <configuration>
                <classifier>javadoc</classifier>
            </configuration>
            <goals>
                <goal>resolve</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Friday, October 16, 2015

Struggling with Travis CI and Maven 3.3

I struggled with fixing Travis CI for my "labs" code. The main problem is lack of support for Maven 3.3; my POM enforcer requires it. Eventually I got something working with help from the Internet, though I'm not happy with it. It manually downloads and uses maven 3.3:

sudo: false
language: java
jdk:
  - oraclejdk8
# TODO: Gross until Travis support setting maven version or upgrades to 3.3
before_install:
  - wget http://apache.claz.org/maven/maven-3/3.3.3/binaries/apache-maven-3.3.3-bin.tar.gz
  - tar zxvf apache-maven-3.3.3-bin.tar.gz
  - chmod +x apache-maven-3.3.3/bin/mvn
  - export M2_HOME=$PWD/apache-maven-3.3.3
  - export PATH=$PWD/apache-maven-3.3.3/bin:${PATH}
  - hash -r
before_script:
  - export M2_HOME=$PWD/apache-maven-3.3.3
  - export PATH=$PWD/apache-maven-3.3.3/bin:${PATH}
  - hash -r
script: mvn verify -Dgpg.skip=true

Bonus

On another project using Travis CI I ran afoul of the 10,000 line limit for displaying build output in the web UI. Workaround, add this to the build_install section:

  - echo 'MAVEN_OPTS="-Dorg.slf4j.simpleLogger.defaultLogLevel=warn"' >~/.mavenrc

If you're using maven 3.3 or better, Karl Heinz Marbaise has a better approach with .mvn/jvm.config.

Thursday, October 15, 2015

Spring advice

Today I had an exchange with an erstwhile colleague, one of those talented few equally comfortable in C++ and Java, great with demanding clients and fellow developers. He asked me for thoughts on Spring dependency injection. Branching out a bit, I replied:

Matter of taste/opinion.

Some things I prefer:

  • Avoid setter injection if at all possible. I want my beans to be finished after DI, not changeable at runtime on accident (very nasty bug to track down) Constructor injection most preferred, followed by field
  • Use standard annotations rather than Spring ones (@Inject), though @Value is needed for injected configuration if you're not using a dedicated configuration object
  • Spring Java configuration beats XML almost all the time
  • If the program doesn't need Spring DI, I prefer using Guice or Dagger. They're simpler, everything is at compile time, and error messages are better. The Spring non-DI libraries play fine with others (e.g., spring-jdbc)
  • Avoid mixing business objects with wiring unless it makes sense. For example, JdbcTemplate should be new'ed as needed, injecting only the DataSource. Injecting the template is an anti-pattern
  • Do use Spring Boot or Dropwizard, et al, if it makes sense. Big time savers, lots of good integration prepackaged
  • For webby programs (apps, services) remember to test unit, controller and integration separately. Spring boot intro page has excellent code examples

Things get more interesting with multiple configurations and with cloud. For example:

  • Do you make separate builds for each env, or one build with multiple configurations? Latter is traditional in EE world, former much better for cloud (devops, immutable, unikernel, etc)
  • Do you pull in configuration externally, e.g., Spring Cloud Config, Netflix Archaius or Apache Zookeeper. I like this approach, but more complex and overkill for simple programs. Nearly mandatory for microservices, and strong choice when in cloud

Did I answer fairly?

Monday, October 12, 2015

Blog code 6

(Updated) I've published my blog code version 6 to Maven Central with javadocs. The previous version was 0.5; having a leading "0." was silly (there won't be a 1.0).

Interesting changes:

  • Added the YAML modules. This is an annotation processor which takes YAML descriptions of Java data structures, and generates immutable classes for them.
  • Added the Matching class for a pattern-matching DSL in Java. I'll improve on over time.

Tuesday, October 06, 2015

Automating dependency versions in Maven

Reading the most excellent Modern Java series of posts, I realized I could automate a command line step I always did manually—updating dependency versions:

$ mvn versions:update-properties

I can automate that! The simplest possible POM demonstrating:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
         http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>example-group</groupId>
    <artifactId>example-artifact</artifactId>
    <version>0-SNAPSHOT</version>

    <properties>
        <example-dependency.version>1</example-dependency.version>
        <generateBackupPoms>false</generateBackupPoms>
        <versions-maven-plugin.version>2.2</versions-maven-plugin.version>
    </properties>

    <dependencies>
        <dependency>
            <groupId>example-group</groupId>
            <artifactId>example-dependency</artifactId>
            <version>${example-dependency.version}</version>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.codehaus.mojo</groupId>
                <artifactId>versions-maven-plugin</artifactId>
                <version>${versions-maven-plugin.version}</version>
                <executions>
                    <execution>
                        <id>update-dependencies</id>
                        <phase>validate</phase>
                        <goals>
                            <goal>update-properties</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
</project>

My hypothetical maven build says:

$ mvn clean
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] Building example-artifact 0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO]
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ example-artifact ---
[INFO]
[INFO] --- versions-maven-plugin:2.2:update-properties (update-dependencies) @ example-artifact ---
[INFO] Property ${example-dependency.version}: Leaving unchanged as 1.2.3
[INFO] Property ${versions-maven-plugin.version}: Leaving unchanged as 2.2

The key is using <phase>validate</phase> in the plugin execution. The first lifecycle phase for maven is "validate". Setting the property "generateBackupPoms" to "false" prevents pom.xml.versionsBackup files from littering the project. You are using source control, aren't you?

Update

This approach is best during development, especially for greenfield projects. I would not recommend it for stable or legacy projects where dependency versions need to be kept constant.

Thursday, September 10, 2015

JUnit @Rule for a SQL transaction

I needed to run some Java test code in a SQL transaction. It modifies the database, changes that should be 1) invisible to other tests, and 2) thrown away after tests finish.

I am using JUnit (of course) so I wrote a @Rule to provide a transaction to my test methods (note I'm using Lombok for convenience):

@RequiredArgsConstructor
public final class SQLTransactionRule
        extends ExternalResource {
    private final DataSource original;
    private final String user;
    private final String password;
    private final Consumer testDataSource;

    private Connection conn;

    public SQLTransactionRule(final DataSource original,
            final Consumer testDataSource) {
        this(original, null, null, testDataSource);
    }

    @Override
    protected void before()
            throws Throwable {
        super.before();
        beginTransaction();
        testDataSource.accept(testDataSource(conn));
    }

    @Override
    protected void after() {
        testDataSource.accept(null);
        rollbackTransaction();
        super.after();
    }

    private void beginTransaction() {
        try {
            final Connection conn = original.getConnection(user, password);
            conn.setAutoCommit(false);
            this.conn = conn;
        } catch (final SQLException e) {
            throw new RuntimeException(
                    format("Cannot create transaction from %s: %s", original,
                            getRootCause(e)), e);
        }
    }

    private void rollbackTransaction() {
        try {
            conn.rollback();
        } catch (final SQLException e) {
            throw new RuntimeException(
                    format("Cannot rollback transaction to %s: %s", original,
                            getRootCause(e)), e);
        }
    }

    private DataSource testDataSource(final Connection conn) {
        return (DataSource) newProxyInstance(getClass().getClassLoader(),
                new Class[]{DataSource.class}, (proxy, method, args) -> {
                    System.out.println("SQLTransactionRule.testDataSource");
                    if (Object.class.equals(method.getDeclaringClass()))
                        return method.invoke(proxy, args);
                    else if ("getConnection".equals(method.getName()))
                        return conn;
                    else
                        return method.invoke(original, args);
                });
    }
}

The motivation for the Consumer of "testDataSource" and for the proxied data source was avoidance of dependencies. Original code has this convenience method, but I decided that was a concern of the test code not the @Rule:

public JdbcTemplate newJdbcTemplate() {
    return new JdbcTemplate(testDataSource);
}

Of course there are tests for the @Rule as well! And this post on Mockito was handy.

@RunWith(MockitoJUnitRunner.class)
public final class SQLTransactionRuleTest {
    @Mock
    private DataSource original;
    @Mock
    private Connection conn;

    @Rule
    public final ExpectedException thrown = ExpectedException.none();

    private SQLTransactionRule rule;
    private DataSource testDataSource;

    @Before
    public void setUp()
            throws SQLException {
        when(original.getConnection(anyString(), anyString())).
                thenReturn(conn);
        rule = new SQLTransactionRule(original,
                testDataSource -> this.testDataSource = testDataSource);
    }

    @Test
    public void shouldExecuteStatement()
            throws Throwable {
        final boolean[] called = {false};
        executeStatement(() -> called[0] = true);
        assertThat(called[0], is(true));
    }

    @Test
    public void shouldPublishTestDataSource()
            throws Throwable {
        executeStatement(
                () -> assertThat(testDataSource, is(notNullValue())));
    }

    @Test
    public void shouldReuseOriginalConnection()
            throws Throwable {
        executeStatement(() -> assertThat(testDataSource.getConnection(),
                is(sameInstance(conn))));
    }

    @Test
    public void shouldTransact()
            throws Throwable {
        executeStatement(() -> verify(conn).setAutoCommit(eq(false)));
        verify(conn).rollback();
    }

    @Test
    public void shouldDescribe()
            throws Throwable {
        thrown.expect(AssertionError.class);
        thrown.expectMessage(containsString("alpha"));
        thrown.expectMessage(containsString("beta"));

        executeStatement(() -> assertThat("alpha", is(equalTo("beta"))));
    }

    @FunctionalInterface
    private interface RunMe {
        void execute()
                throws Throwable;
    }

    private void executeStatement(final RunMe statement)
            throws Throwable {
        rule.apply(new Statement() {
            @Override
            public void evaluate()
                    throws Throwable {
                statement.execute();
            }
        }, EMPTY).evaluate();
    }
}

Saturday, June 27, 2015

REST API best practices

Vinay Sahni of Enchant writes Best Practices for Designing a Pragmatic RESTful API, a complete, current best practices article on REST APIs, chock full of explanation, examples and real APIs from top web sites. Enchant itself is a good example how to document a REST API. Vinay has been writing a long series of articles on REST APIs, plenty of chewy links for further reading. Main points:

  • Key requirements for the API
  • Use RESTful URLs and actions
  • SSL everywhere - all the time
  • Documentation
  • Versioning
  • Result filtering, sorting & searching
  • Limiting which fields are returned by the API
  • Updates & creation should return a resource representation
  • Should you HATEOAS?
  • JSON only responses
  • snake_case vs camelCase for field names
  • Pretty print by default & ensure gzip is supported
  • Don't use an envelope by default, but make it possible when needed
  • JSON encoded POST, PUT & PATCH bodies
  • Pagination
  • Auto loading related resource representations
  • Overriding the HTTP method
  • Rate limiting
  • Authentication
  • Caching
  • Errors
  • HTTP status codes
  • In Summary

Friday, June 26, 2015

Pattern matching for Java

Java does not (yet) have pattern matching although writing a legible DSL for it is not too hard. I'm certainly not the first to take a hand at it.

An example DSL in action. Lambdas and method references sure help a lot:

public static void main(final String... args) {
    asList(0, 1, 2, 3, 13, 14, null, -1).stream().
            peek(n -> out.print(format("%d -> ", n))).
            map(matching(Integer.class, Object.class).
                    when(Objects::isNull).then(n -> "!").
                    when(is(0)).then(nil()).
                    when(is(1)).then("one").
                    when(is(13)).then(() -> "unlucky").
                    when(is(14)).then(printIt()).
                    when(even()).then(scaleBy(3)).
                    when(gt(2)).then(dec()).
                    none().then("no match")).
            map(MatchingTest::toString).
            forEach(out::println);
    out.flush(); // Avoid mixing sout and serr

    matching(Integer.class, Void.class).
            none().thenThrow(RuntimeException::new).
            apply(0);
}

Implementation:

/**
 * {@code Matching} represents <a href="https://en.wikipedia.org/wiki/Pattern_matching">Pattern
 * Matching</a> in Java as a function production an optional.  Example: <pre>
 * asList(0, 1, 2, 3, 13, 14, null, -1).stream().
 *         peek(n -> out.print(format("%d -> ", n))).
 *         map(matching(Integer.class, Object.class).
 *             when(Objects::isNull).then(n -&gt; "!").
 *             when(is(0)).then(nil()).
 *             when(is(1)).then("one").
 *             when(is(13)).then(() -&gt; "unlucky").
 *             when(is(14)).then(printIt()).
 *             when(even()).then(scaleBy(3)).
 *             when(gt(2)).then(dec()).
 *             none().then("no match")).
 *         map(MatchingTest::toString).
 *         forEach(out::println);</pre>
 * <p>
 * <i>NB</i> &mdash; There is no way to distinguish from an empty optional if
 * there was no match, or if a match mapped the input to {@code null}, without
 * use of a {@link When#then(Object) sentinel value} or {@link
 * When#thenThrow(Supplier) thrown exception}.
 * <p>
 * <strong>NB</strong> &mdash; There is no formal destructuring, but this can
 * be simulated in the {@code Predicate} to {@link #when(Predicate) when}.
 *
 * @param <T> the input type to match against
 * @param <U> the output type of a matched pattern
 *
 * @author <a href="mailto:binkley@alumni.rice.edu">B. K. Oxley (binkley)</a>
 */
@NoArgsConstructor(access = PRIVATE)
public final class Matching<T, U>
        implements Function<T, Optional<U>> {
    private final Collection<Case> cases = new ArrayList<>();

    /**
     * Begins pattern matching with a new pattern matcher.
     *
     * @param inType the input type token, never {@code null}
     * @param outType the output type token, never {@code null}
     * @param <T> the input type to match against
     * @param <U> the output type of a matched pattern
     *
     * @return the pattern matcher, never {@code null}
     *
     * @todo Avoid the type tokens
     */
    public static <T, U> Matching<T, U> matching(final Class<T> inType,
            final Class<U> outType) {
        return new Matching<>();
    }

    /**
     * Begins a when/then pair.
     *
     * @param when the pattern matching test, never {@code null}
     *
     * @return the pattern continuance, never {@code null}
     */
    public When when(final Predicate<? super T> when) {
        return new When(when);
    }

    /**
     * Begins a default when/then pair, always placed <strong>last</strong> in
     * the list of cases (evaluates no cases after this one).
     *
     * @return then pattern continuance, never {@code null}
     */
    public When none() {
        return when(o -> true);
    }

    /**
     * Evaluates the pattern matching.
     *
     * @param in the input to match against, possibly {@code null}
     *
     * @return the match result (empty if no match), never {@code null}
     */
    @Override
    public Optional<U> apply(final T in) {
        return cases.stream().
                filter(c -> c.p.test(in)).
                findFirst().
                map(c -> c.q.apply(in));
    }

    @RequiredArgsConstructor(access = PRIVATE)
    public final class When {
        /**
         * Number of frames to discard when creating an exception for a match.
         * Very sensitive to implementation.  This aids in understanding stack
         * traces from matching, discarding internal machinery and leaving the
         * actual throwing call at the top of the stack.
         */
        private static final int N = 7;
        private final Predicate<? super T> when;

        /**
         * Ends a when/then pair, evaluating <var>then</var> against the input
         * if matched.
         *
         * @param then the pattern matching function, never {@code null}
         *
         * @return the pattern matcher, never {@code null}
         */
        public Matching<T, U> then(
                final Function<? super T, ? extends U> then) {
            cases.add(new Case(when, then));
            return Matching.this;
        }

        /**
         * Ends a when/then pair, returning <var>then</var> if matched.
         *
         * @param then the pattern matching value, possibly {@code null}
         *
         * @return the pattern matcher, never {@code null}
         */
        public Matching<T, U> then(final U then) {
            cases.add(new Case(when, x -> then));
            return Matching.this;
        }

        /**
         * Ends a when/then pair, evaluating <var>then</var> independent of
         * supplier if matched.
         *
         * @param then the pattern matching supplier, never {@code null}
         *
         * @return the pattern matcher, never {@code null}
         */
        public Matching<T, U> then(final Supplier<? extends U> then) {
            cases.add(new Case(when, x -> then.get()));
            return Matching.this;
        }

        /**
         * Ends a when/then pair, evaluating <var>then</var> to {@code null}
         * if matched.
         *
         * @param then the input consumer, never {@code null}
         *
         * @return the pattern matcher, never {@code null}
         */
        public Matching<T, U> then(final Consumer<? super T> then) {
            cases.add(new Case(when, o -> {
                then.accept(o);
                return null;
            }));
            return Matching.this;
        }

        /**
         * Ends a when/then pair, evaluating <var>then</var> independent of
         * supplier and throwing the new exception if matched.
         *
         * @param then the pattern matching exception supplier, never {@code
         * null}
         *
         * @return the pattern matcher, never {@code null}
         */
        public Matching<T, U> thenThrow(
                final Supplier<RuntimeException> then) {
            cases.add(new Case(when, x -> {
                final RuntimeException e = then.get();
                final List<StackTraceElement> stack = asList(
                        e.getStackTrace());
                e.setStackTrace(stack.subList(N, stack.size()).
                        toArray(new StackTraceElement[stack.size() - N]));
                throw e;
            }));
            return Matching.this;
        }
    }

    @RequiredArgsConstructor(access = PRIVATE)
    private final class Case {
        private final Predicate<? super T> p;
        private final Function<? super T, ? extends U> q;
    }
}

UPDATE: I recently found the very nice Derive4J annotation processor library which provides similar functionality in the form of Algebraic Data Types (ADTs), aka, Sum Types in functional languages.

Thursday, May 28, 2015

RESTful errors, Simple Boot

Handling Errors

Reading Travis McChesney's RESTful API Design Part III: Error Handling, there are more ways to represent errors in REST APIs than the returned body. A toy API I am writing as a playground for these idea is named Simple Boot, a jest on "Spring Boot". There I have an "X-Correlation-ID" header for tracking calls through services.

  • Should "X-Correlation-ID" be required?
  • Should it be supplied automatically?
  • When required and missing, what should the caller receive?

Answering the third the caller sees:

HTTP/1.1 400 Bad Request
Server: Apache-Coyote/1.1
Warning: 250 localhost:8080 "Missing X-Correlation-ID header"
Content-Type: application/json;charset=UTF-8
Transfer-Encoding: chunked
Date: Thu, 28 May 2015 12:46:40 GMT
Connection: close

{
  "timestamp": 1432817125588,
  "status": 400,
  "error": "Bad Request",
  "message": "Missing X-Correlation-ID header",
  "path": "/hello/Bob"
}

Here rather than choosing among options, I take them all:

  1. Return a 400 for a bad request
  2. Describe the error in the response body
  3. Add a header describing the error

As McChesney points out, there is not general agreement on reporting errors. For example, Facebook returns 200 for errors, requiring parsing of the response body.

Using the "Warning" header for this purpose is uncommon but supported in the specification albeit obliquely. However not everyone agrees. I use warning code 250, one I made up. The 1xx codes are transient errors; the 2xx codes permanent. In another example I use a 1xx code for an upstream service currently unavailable, and return cached data, which is closer to the described uses of "Warning".

Other header options:

  • Using a custom code outside 1xx or 2xx with "Warning". This makes sense for in-house services, may cause issues with caching devices but unclear
  • Use a custom HTTP header, a common solution, again may have issues with intermediate devices

Some Spring Bonus

Simple Boot has been fun and instructive. One Spring Boot feature I stumbled on is automated binding of configuration properties. For services requiring "X-Correlation-ID" I configure with @ConfigurationProperties(prefix = "headers.correlation-id.server"). To automate clients I use @ConfigurationProperties(prefix = "headers.correlation-id.client").

As an example of solving one issue with headers, I write Feign pass-through support.

Of course there are tests.

Sunday, March 15, 2015

Dynamic properties in Java

Motivation

I've been looking at the problem of dynamic properties in Java. Typically properties are injected into a program at start and never change. Common examples include timeouts, host names, etc. Changing these requires and edit and restart (edit can also mean an update to a remote data source).

What I want is runtime updates to properties, and the program uses the new values. Hence "dynamic properties".

There are several schemes for this built around frameworks or external data sources. I want something using only the JDK.

Interfaces

My interfaces became one for tracking key-value pairs, one for updating them (javadoc munged to display in post):

/**
 * Tracking tracks string key-value pairs, tracking external updates
 * to the boxed values.  Optionally tracks them as a type converted from
 * string.  Example use: 
 * Tracking dynafig = ...;
 * Optional<AtomicReference<String>> prop = dynafig.track("prop");
 * boolean propDefined = prop.isPresent();
 * AtomicReference<String> propRef = prop.get();
 * String propValue = propRef.get();
 * // External source updates key-value pair for "prop"
 * String newPropValue = propRef.get();
* * In an injection context:
 * class Wheel {
 *     private final AtomicInteger rapidity;
 *
 *     @Inject
 *     public Wheel(final Tracking dynafig) {
 *         rapidity = dynafig.track("rapidity").
 *                 orElseThrow(() -> new IllegalStateException(
 *                         "Missing 'rapidity' property));
 *     }
 *
 *     public void spin() {
 *         spinAtRate(rapidity.get());
 *     }
 * }
* * @author Brian Oxley * @see Updating Updating key-value pairs * @see Default Reference implementation */ public interface Tracking { /** * Tracks the given key value as a string. Returns empty if * key is undefined. If key is defined, may stil * return a null boxed value. * * @param key the key, never missing * * @return the optional atomic value string, never missing * * @todo Some way to work out type from convert */ @Nonnull Optional<AtomicReference<String>> track(@Nonnull final String key); /** * Tracks the given key value as a boolean. Returns empty if * key is undefined. * * @param key the key, never missing * * @return the optional atomic value boolean, never missing */ @Nonnull Optional<AtomicBoolean> trackBool(@Nonnull final String key); /** * Tracks the given key value as an integer. Returns empty if * key is undefined. * * @param key the key, never missing * * @return the optional atomic value integer, never missing */ @Nonnull Optional<AtomicInteger> trackInt(@Nonnull final String key); /** * Tracks the given key value as type. Returns * empty if key is undefined. If key is defined, * may stil return a null boxed value. * * @param key the key, never missing * @param type the value type token, never missing * @param convert the value converter, never missing * @param <T> the value type * * @return the optional atomic value reference, never missing * * @todo Some way to work out type from convert */ @Nonnull <T> Optional<AtomicReference<T>> trackAs(@Nonnull final String key, @Nonnull final Class<T> type, // TODO: Can this be worked out? @Nonnull final Function<String, T> convert); }
/**
 * {@code Updating} updates key-value pairs.
 *
 * @author Brian Oxley
 * @see Tracking Tracking key-value pairs
 * @see Default Reference implementation
 */
public interface Updating {
    /**
     * Updates a key-value pair with a new value.
     *
     * @param key the key, never missing
     * @param value the value, possibly null
     *
     * @return true if updated else false if created
     */
    boolean update(@Nonnull final String key, @Nullable final String value);

    /**
     * Updates a key-value pair as a map entry for convenience.
     *
     * @param entry the entry, never missing
     *
     * @return true if updated else false if created
     * @see #update(String, String)
     */
    default boolean update(@Nonnull final Map.Entry<String, String> entry) {
        return update(entry.getKey(), entry.getValue());
    }

    /**
     * Updates a set of key-value pairs for convenience.  Each key is
     * invidually updated in entry-set order.
     *
     * @param values the key-value set, never missing
     *
     * @see #update(String, String)
     */
    default void updateAll(@Nonnull final Map<String, String> values) {
        values.entrySet().stream().
                forEach(this::update);
    }
}

Team lead

I built a reference implementation to demonstrate these were plausible. To my team I explained like this:

  • Coded very much in Java 8 functional style
  • Trivial API - 4 tracking calls (variants on return type), 1 updating call (plus 2 convenience)
  • Minimal dependencies - JDK + javax.annotations (@Nonnull)
  • Thread-safe, not concurrency-safe (lost updates, etc). Fine if properties are not updated on top of each other (i.e., several in the same microsecond)
  • I expect it to be straight-forward to integrate into JSR107, Cassandra, etc, but I've not tried this from home

And offered some advice to my juniors:

  • Be functional where sensible, the benefits are almost beyond enumeration
  • Corollary: Avoid state - in Java that means fields. Temp variables (locals) are debatable and come down to individual taste. I lean to avoiding them, but there's nothing wrong with creating locals that clarify the code for others (including yourself 6 mos. later)
  • Corollary: Push fields down as low as possible, avoid globals and "local globals" (static fields)
  • Complicate your data structure, not your calls. People reason about complex data structures much better than they do about complex logic
  • Avoid null. Really. Treat all uses of null as code smell - think of it as "machine level" programming. You should do high-level programming

Implementation

All tests pass. The reference implementation:

public final class Default
        implements Tracking, Updating {
    private final Map<String, Value> keys = new ConcurrentHashMap<>();
    private final Map<String, Value> values = new ConcurrentHashMap<>();

    public Default() {
    }

    public Default(@Nonnull final Map<String, String> keys) {
        updateAll(keys);
    }

    @Nonnull
    @Override
    public Optional<AtomicReference<String>> track(
            @Nonnull final String key) {
        return Optional.ofNullable(keys.get(key)).
                map(Value::get);
    }

    @Nonnull
    @Override
    public Optional<AtomicBoolean> trackBool(@Nonnull final String key) {
        return Optional.ofNullable(keys.get(key)).
                map(Value::getBool);
    }

    @Nonnull
    @Override
    public Optional<AtomicInteger> trackInt(@Nonnull final String key) {
        return Optional.ofNullable(keys.get(key)).
                map(Value::getInt);
    }

    @Nonnull
    @Override
    public <T> Optional<AtomicReference<T>> trackAs(@Nonnull final String key,
            @Nonnull final Class<T> type,
            @Nonnull final Function<String, T> convert) {
        return Optional.ofNullable(keys.get(key)).
                map(v -> v.getAs(type, convert));
    }

    @Override
    public boolean update(@Nonnull final String key,
            @Nullable final String value) {
        return null != keys.put(key, values.compute(key,
                (k, v) -> null == v ? new Value(value) : v.update(value)));
    }

    private static final class Atomic<T> {
        private final T atomic;
        private final Consumer<String> update;

        private static Atomic<AtomicReference<String>> of(
                final String value) {
            final AtomicReference<String> atomic = new AtomicReference<>(
                    value);
            return new Atomic<>(atomic, atomic::set);
        }

        private static Atomic<AtomicBoolean> boolOf(final String value) {
            final AtomicBoolean atomic = new AtomicBoolean(
                    null == value ? false : Boolean.valueOf(value));
            return new Atomic<>(atomic,
                    v -> atomic.set(null == v ? false : Boolean.valueOf(v)));
        }

        private static Atomic<AtomicInteger> intOf(final String value) {
            final AtomicInteger atomic = new AtomicInteger(
                    null == value ? 0 : Integer.valueOf(value));
            return new Atomic<>(atomic,
                    v -> atomic.set(null == v ? 0 : Integer.valueOf(v)));
        }

        private static <T> Atomic<AtomicReference<T>> asOf(final String value,
                final Function<String, T> convert) {
            final AtomicReference<T> atomic = new AtomicReference<>(
                    convert.apply(value));
            return new Atomic<>(atomic, v -> atomic.set(convert.apply(v)));
        }

        private Atomic(final T atomic, final Consumer<String> update) {
            this.atomic = atomic;
            this.update = update;
        }

        private void update(final String value) {
            update.accept(value);
        }
    }

    private final class Value {
        @Nullable
        private final String value;
        private final Map<Class<?>, Atomic<?>> values;

        private Value(final String value) {
            this(value, new ConcurrentHashMap<>(3));
        }

        private Value(@Nullable final String value,
                final Map<Class<?>, Atomic<?>> values) {
            this.value = value;
            this.values = values;
        }

        private Value update(final String value) {
            final Value newValue = new Value(value, values);
            newValue.values.values().stream().
                    forEach(a -> a.update(value));
            return newValue;
        }

        @SuppressWarnings("unchecked")
        private AtomicReference<String> get() {
            return (AtomicReference<String>) values.
                    computeIfAbsent(String.class,
                            k -> Atomic.of(value)).atomic;
        }

        private AtomicBoolean getBool() {
            return (AtomicBoolean) values.
                    computeIfAbsent(Boolean.class,
                            k -> Atomic.boolOf(value)).atomic;
        }

        private AtomicInteger getInt() {
            return (AtomicInteger) values.
                    computeIfAbsent(Integer.class,
                            k -> Atomic.intOf(value)).atomic;
        }

        @SuppressWarnings("unchecked")
        private <T> AtomicReference<T> getAs(final Class<T> type,
                final Function<String, T> convert) {
            return (AtomicReference<T>) values.
                    computeIfAbsent(type,
                            k -> Atomic.asOf(value, convert)).atomic;
        }

        @Override
        public boolean equals(final Object o) {
            if (this == o)
                return true;
            if (o == null || getClass() != o.getClass())
                return false;
            final Value that = (Value) o;
            return Objects.equals(value, that.value);
        }

        @Override
        public int hashCode() {
            return Objects.hash(value);
        }
    }
}

Wednesday, March 04, 2015

Does refactoring worsen code?

Thought provoking: Study finds that refactoring doesn’t improve code quality. The opening:

Refactoring software, that is, restructuring existing source code to make it more readable, efficient, and maintainable, is something all developers do every now and again. Of course, the implicit assumption behind refactoring is that the benefits (time and headaches saved in the future) outweigh the costs (time and effort spent now). However, new experimental research suggests that this may not be the case and that software code quality may not be improved much, if at all, by refactoring.

The study was done by researchers in Sri Lanka and recently published in the International Journal of Software Engineering & Applications titled An Empirical Evaluation of Impact of Refactoring On Internal and External Measures of Code Quality. The goal was to test whether common refactoring techniques resulted in measurable improvements in software quality, both externally (e.g., Is the code more maintainable?) and internally (e.g., Number of lines of code).

The researchers selected a small-scale application (about 4,500 lines of C# code) used by the academic staff at the University of Kelaniya for scheduling events and managing online documents for evaluation. 10 common refactoring techniques were applied to the code (e.g., Replace Type Code with Subclasses, Replace Conditional with Polymorphism).

Reading the press account I expect objections such as:

C#?
Well, language should not matter here
Too small an example!
This is a fair complaint, the impact of refactoring scales with code base size
Those weren't professionals
The reviewers were a mix, but this complaint overlooks how much code is written and maintained by "non-programmers", an oxymoron as programming is an activity, not a type of person

My own objection is that the refactoring did not go far enough. Deepening a type hierarchy does often make code "worse", delegation is often a better choice than inheritance. In a larger code base than the sample used, this become more obvious.

Friday, February 27, 2015

Architects, engineers?

Delightful passage from Sam Newman's new book, Building Microservices:

Part of us wants recognition, so we borrow names from other professions that already have the recognition we as an industry crave. But this can be doubly harmful. First, it implies we know what we are doing, when we plainly don’t. I wouldn’t say that buildings and bridges never fall down, but they fall down much less than the number of times our programs will crash, making comparisons with engineers quite unfair. Second, the analogies break down very quickly when given even a cursory glance. To turn things around, if bridge building were like programming, halfway through we’d find out that the far bank was now 50 meters farther out, that it was actually mud rather than granite, and that rather than building a footbridge we were instead building a road bridge. Our software isn’t constrained by the same physical rules that real architects or engineers have to deal with, and what we create is designed to flex and adapt and evolve with user requirements.

A lot of times I really don't understand my own job.

Sunday, February 01, 2015

Dice, Test First and saving time

tl;dr — Test first saves time. Also a dice expression calculator.

The setup

Larry Wall famously remarked, "the three great virtues of a programmer: laziness, impatience, and hubris." A skill earned by good programmers is knowing when to favor which virtue. I failed while writing a parser for "dice expressions" ala Dungeons & Dragons. (Example: "3d6 + 1" means roll 3 6-sided dice, sum the results and add 1.)

The parser is straight-forward using the popular ANTLR4 tools:

grammar DiceExpression;

expr: left=expr op=('*' | '/') right=expr #opExpr
    | left=expr op=('+' | '-') right=expr #opExpr
    | '(' group=expr ')' #groupExpr
    | van=('adv' | 'dis')? number=NUMBER? sides=SIDES #diceExpr
    | number=NUMBER #numberExpr
    ;

dice: number=NUMBER? sides=SIDES;

NUMBER: '1'..'9' '0'..'9'*;
SIDES: 'd' ('4' | '6' | '8' | '10' | '12' | '20' | '100')
    { setText(getText().substring(1)); };

WS: [ \t]+ -> skip;

The grammar supports a 5th Edition feature, Advantage/Disadvantage—where you get to roll twice and pick the better/worse result—, with the keywords adv/dis. A less-5th Edition-specific grammar would leave out that term from "#diceExpr".

Enter the rube

All tests pass: good! Then I thought, "What about whitespace in dice rolls or with adv/dis?" I was concerned with expressions such as "1 +disd6" or "3 d4". Surely those are invalid, but I don't check for them. So I began adding explicit whitespace parsing in the #diceExpr rule. Mistake!

Several of my tests failed for perfectly good dice expressions.

Okay, so I reverted my grammar changes. To understand what was going on, I added tests I should have started with before editing (lines marked "FAIL"):

@RunWith(Parameterized.class)
public final class DiceExpressionTest {
    @Parameters(name = "{index}: '{0}'")
    public static Collection<Object> parameters() {
        return asList(args("1", true),
                args("d6", true),
                args("3d6", true),
                args("1+3d6", true),
                args("adv d6", true),
                args("dis 3d6", true),
                args("disd6", false), // FAIL
                args("1 + 3 d6", false)); // FAIL
    }

    private final ExpectedException thrown = ExpectedException.none();

    @Parameter(0)
    public String in;
    @Parameter(1)
    public boolean ok;

    @Test
    public void shouldParse() {
        if (!ok)
            thrown.expect(ParseCancellationException.class);

        // Just check parsing works, ignore results
        DiceExpression.valueOf(in).evalWith(Roll.die());
    }

    private static Object[] args(final Object... args) {
        return args;
    }
}

And ... the "FAIL" tests pass with the original grammar. This got me to thinking more about how ANTLR works. It is a lexer/parser. The lexer grabs strings from the input and hands them to the parser to figure out. The lexer has no brains, it's an expert at chopping up input. (Great post on this.)

The cases I worried about? The lexer does not know how to chop up the first one ("disd6"): it matches no token pattern. The parser does not understand the tokens in the second one ("1 + 3 d6"): it expects an arithmetic operator between "3" and "d6". My grammar already did the right thing.

Had I started with tests for these cases, instead of jumping right to coding, I would have not spent time messing with whitespace in the grammar. Coding first cost time rather than saved it.

Enlightenment by coding Kōan

Write tests first! No, really. Even when you "know" the solution, write tests first. There are many ways it saves time in the long run, it helps you think about your API, and it can save time in the short run. My dice expression example is not unique: test-first avoids unneeded work.

Epilogue

What does the dice expression implementation look like?

public final class DiceExpression {
    private final ExprContext expression;

    @Nonnull
    public static DiceExpression valueOf(@Nonnull final String expression) {
        final DiceExpressionLexer lexer = new DiceExpressionLexer(
                new ANTLRInputStream(expression));
        final DiceExpressionParser parser = new DiceExpressionParser(
                new CommonTokenStream(lexer));
        parser.setErrorHandler(new BailErrorStrategy());

        return new DiceExpression(parser.expr());
    }

    private DiceExpression(final ExprContext expression) {
        this.expression = expression;
    }

    public int evalWith(final Roll roll) {
        return expression.accept(new EvalWithVisitor(roll));
    }

    private static final class EvalWithVisitor
            extends DiceExpressionBaseVisitor<Integer> {
        private final Roll roll;

        public EvalWithVisitor(final Roll roll) {
            this.roll = roll;
        }

        @Override
        public Integer visitOpExpr(final OpExprContext ctx) {
            final int left = visit(ctx.left);
            final int right = visit(ctx.right);
            switch (ctx.op.getText().charAt(0)) {
            case '+': return left + right;
            case '-': return left - right;
            case '*': return left * right;
            case '/': return left / right;
            }
            throw new ArithmeticException(
                    "Unknown operator: " + ctx.getText());
        }

        @Override
        public Integer visitGroupExpr(final GroupExprContext ctx) {
            return visit(ctx.group);
        }

        @Override
        public Integer visitDiceExpr(final DiceExprContext ctx) {
            return Dice.valueOf(ctx).evalWith(roll);
        }

        @Override
        public Integer visitNumberExpr(final NumberExprContext ctx) {
            return Integer.valueOf(ctx.number.getText());
        }
    }
}

And Dice, overly complicated until I can think through Advantage/Disadvantage further:

public class Dice
        implements Comparable<Dice> {
    @FunctionalInterface
    public interface Roll {
        int roll(final int sides);

        static Roll die() {
            final Random random = new Random();
            return sides -> random.nextInt(sides) + 1;
        }
    }

    public enum Advantage {
        DIS(2, Math::min),
        NONE(1, (a, b) -> a),
        ADV(2, Math::max);

        private final int rolls;
        private final IntBinaryOperator op;

        Advantage(final int rolls, final IntBinaryOperator op) {
            this.rolls = rolls;
            this.op = op;
        }

        public final int evalWith(final Roll roll, final Dice dice) {
            return range(0, rolls).
                    map(n -> dice.rollWith(roll)).
                    reduce(op).
                    getAsInt();
        }

        public final String toString(final Dice dice) {
            switch (this) {
            case NONE:
                return dice.stringOf();
            default:
                return name().toLowerCase() + ' ' + dice.stringOf();
            }
        }
    }

    private final Optional<Integer> number;
    private final int sides;
    private final Advantage advantage;

    @Nonnull
    public static Dice valueOf(@Nonnull final String expression) {
        final DiceExpressionLexer lexer = new DiceExpressionLexer(
                new ANTLRInputStream(expression));
        final DiceExpressionParser parser = new DiceExpressionParser(
                new CommonTokenStream(lexer));
        parser.setErrorHandler(new BailErrorStrategy());

        return valueOf((DiceExprContext) parser.expr());
    }

    @Nonnull
    static Dice valueOf(@Nonnull final DiceExprContext ctx) {
        final Optional<Integer> number = Optional.ofNullable(ctx.number).
                map(Token::getText).
                map(Integer::valueOf);
        final Integer sides = Integer.valueOf(ctx.sides.getText());
        final Advantage advantage = Optional.ofNullable(ctx.van).
                map(Token::getText).
                map(String::toUpperCase).
                map(Advantage::valueOf).
                orElse(NONE);
        return new Dice(number, sides, advantage);
    }

    public Dice(@Nonnull final Optional<Integer> number, final int sides,
            final Advantage advantage) {
        this.number = number;
        this.sides = sides;
        this.advantage = advantage;
    }

    public Dice(@Nullable final Integer number, final int sides,
            final Advantage advantage) {
        this(Optional.ofNullable(number), sides, advantage);
    }

    public Dice(@Nullable final Integer number, final int sides) {
        this(Optional.ofNullable(number), sides, NONE);
    }

    public int number() {
        return number.orElse(1);
    }

    public int sides() {
        return sides;
    }

    public Advantage advantage() {
        return advantage;
    }

    public int evalWith(final Roll roll) {
        return advantage.evalWith(roll, this);
    }

    protected int rollWith(final Roll roll) {
        return rangeClosed(1, number()).
                map(ignored -> roll.roll(sides())).
                sum();
    }

    @Override
    public int compareTo(@Nonnull final Dice that) {
        // Sort by roll average - adv/dis messes this up though
        final int compare = Integer.compare(number() + (sides() + 1),
                that.number() * (that.sides() + 1));
        return 0 == compare ? advantage().compareTo(that.advantage())
                : compare;
    }

    @Override
    public String toString() {
        return advantage.toString(this);
    }

    protected String stringOf() {
        return number.
                map(number -> number + "d" + sides()).
                orElse("d" + sides());
    }

    @Override
    public boolean equals(final Object o) {
        if (this == o)
            return true;
        if (o == null || getClass() != o.getClass())
            return false;

        final Dice that = (Dice) o;

        return sides() == that.sides() && number() == that.number()
                && advantage() == that.advantage();
    }

    @Override
    public int hashCode() {
        return 31 * (31 * sides() + number()) * advantage().hashCode();
    }
}
UPDATE: If you wonder about the parser visitor, you have to explicitly enable it in ANTLR4 (even though it is recommended over tree rewriting). Use the command line flag, or in Maven

<plugin>
    <groupId>org.antlr</groupId>
    <artifactId>antlr4-maven-plugin</artifactId>
    <version>${antlr4.version}</version>
    <configuration>
        <visitor>true</visitor>
    </configuration>
    <executions>
        <execution>
            <id>antlr4</id>
            <goals>
                <goal>antlr4</goal>
            </goals>
        </execution>
    </executions>
</plugin>