Dirty Code Monday!

Lately I’ve been thinking about how easy it is to fall into the trap of not challenging our ideas about the code we’re working on. In order to challenge the default mindset of Clean Code, I recently proposed to institute Dirty Code Monday (a proposal that sort of got me into a bit of a big discussion).

Anyway, here is the report from the first successful Dirty Code Monday one week ago:

An unpromising start: I had done a code review with “Programmer A”’s code on Thursday where “Programmer H” and I had pretty much rewritten all of it. Now we had to go through the results together. We all admitted that code review is not ideal for a Dirty Code Monday, but rules are rules.

The first bit of code we came across was this:

public static void setupClientCertificate(HttpURLConnection httpURLConnection) throws IOException {
    if (isUsingClientCertificates()) {
        HttpsURLConnection httpsURLConnection = (HttpsURLConnection) httpURLConnection;
        try {
            // TODO: Find out if SSLContext.getSocketFactory is expensive an if so, cache
            httpsURLConnection.setSSLSocketFactory(createSocketFactory(keyStorePath(), keystorePassword(), trustStorePath()));
        } catch (KeyStoreException | CertificateException | NoSuchAlgorithmException | UnrecoverableKeyException | KeyManagementException e) {
            throw new RuntimeException(e);
        }
    }
}

Notice the comment? // TODO: Find out if SSLContext.getSocketFactory is expensive an if so, cache. I sighed. “We really have to remember not to do premature optimization, or we’ll never find out if it is really slow or not.” “Programmer A” responded “What? I thought you said this was Dirty Code Monday! Let’s just do it!” And so we did. We cached the result of createSocketFactory in a static variable. Dirty Code Monday!

By now, I was starting to get my spirits up that we could actually pull this off. We glanced up at the on-wall monitor: Someone had broken the build. “Sorry,” said ‘Programmer K’, “I forgot that my commit also changed the backend.” ‘Programmer R’ chimed in: “Yay! Your turn to get cake, K”. “Not today,” said A, “Dirty Code Monday!”

‘Project manager K’ just mumbles “The cake is a lie”.

I continued the code review with A. Next up was this beauty:

private boolean isValidRequest(HttpServletRequest req) {
    List validPaths = req.getMethod().equals("GET") ? Configuration.validGetPaths() : Configuration.validPostPaths();
    return req.getPathInfo() != null && validPaths.stream().anyMatch(req.getPathInfo()::startsWith);
}

“Hmm…” I said, “I guess that really should deal with DELETE and PUT requests, too. But it’s Dirty Code Monday.” We discussed a little and A mused “a switch-case could work here”. “Ugh! I say, I don’t like switch-statements. They kinda violate the OCP. Maybe. Maybe not here. But I still don’t like it.” He just looked at me and we both laughed.

private boolean isValidRequest(HttpServletRequest req) {
    if (req.getPathInfo() == null) return false;
    switch (req.getMethod()) {
        case "GET":
            return Configuration.validGetPaths().stream().anyMatch(req.getPathInfo()::startsWith);
        case "POST":
            return Configuration.validPostPaths().stream().anyMatch(req.getPathInfo()::startsWith);
        default:
            return false;
    }
}

“Ooooh,” I said. “Look at that repetition of the complicated stream() expression. Perfect!” ‘Programmer A’ was pleased as well “Dirty Code Monday is pretty okay.”

Perhaps you think our code wasn’t that dirty. I’m actually not that embarrassed about it. But changing our perspective helped us see other ways of working with the code than we were used to. Having a phrase like “Dirty Code Monday” was a fun thing for the team to talk about. I’d like to try it again today.

Happy Dirty Code Monday, everybody!

PS: The code in this article is actual production code. The whole team has approved the quotes attributed to them