2
\$\begingroup\$

Advent of Code 2018, day 2, part 2:

The [two IDs you are looking for] differ by exactly one character at the same position in both strings. For example, given the following IDs:

abcde
fghij
klmno
pqrst
fguij
axcye
wvxyz

The IDs fghij and fguij differ by exactly one character, the third (h and u). Those must be the correct IDs.

The code checks a list of strings to find two that match apart from a character at one position. So "abc" and "abz" would match.

I'm very green with Clojure, so feedback about any aspect of this code is appreciated.

(require 'clojure.set)

(def not-empty? (complement empty?))

(def input-lines (line-seq (clojure.java.io/reader "input.in")))

;; Given a string
;; Return a set of similar strings, with an asterisk replacing each character
;; eg "abc" gives #{[\* b c] [a \* c] [a b \*]}
(defn get-match-patterns [id]
  (set (map #(assoc (vec id) % \*) (range (count id)))))

((fn [ids seen-patterns]
  (when (empty? ids)
    (throw (Exception. "Didn't find match")))

  (let [[next-id & other-ids] ids
        match-patterns (get-match-patterns next-id)
        matches (clojure.set/intersection match-patterns seen-patterns)]
    (if (not-empty? matches)
      (prn (apply str (filter #(not= \* %) (first matches))))
      (recur other-ids (apply conj seen-patterns match-patterns))))) input-lines #{})

Sample input:

abcde
fghij
klmno
pqrst
fguij
axcye
wvxyz

Sample output:

fgij
\$\endgroup\$
3
\$\begingroup\$

There isn't a whole lot of code here, so I'll just point out what I see top-to-bottom. I'll note though that I'm exceedingly tired and don't fully understand what the problem you're solving is, so I'll just be commenting on aspects of language usage:


Your file starts as (require 'clojure.set). Every Clojure file should start with a call to the ns macro that creates a namespace matching your file's position under the source file of the project. If this file was at PROJECTFOLDER/src/subfolder/my_file.clj, the file should start with

(ns subfolder.my-file)

This allows your file to be properly included by other files.

You have a call to require also though. This can neatly be added to the ns call:

(ns subfolder.my-file
  (:require [clojure.set]))

You aren't even taking advantage of the require though; you can call clojure.set/intersection even without the require. With the require, you can write set/intersection. That's not as nice as it could be though. If you use :as, you can shorten the uses further:

(ns subfolder.my-file
  (:require [clojure.set :as s])) ; Aliasing the set namespace as s

(s/intersection ...) ; Works!

(def not-empty? (complement empty?))

You seem to already be familiar with functional languages. Do you know Haskell? While it's certainly OK to write this function like this, as a complement of another, after a lot of messing around, I've found it's better to just write it out fully in most cases:

(defn not-empty? [coll]
  (not (empty? coll)))

Why? Because now when you ctrl+q it in a good IDE, you get a nice documentation summary that includes the parameter names. For a case like this, most people will immediately know what not-empty? does. For more complicated cases though, you may find that writing out the full defn leads to easier to understand code and generated documentation. Remember the mantra: "code is read more often than it's written". Don't make code harder to understand just to save a few keystrokes.

Really though, do you need this at all? (not (empty? coll)) is arguably just as readable as (not-empty? coll). You also only use it once, and can simply reverse the bodies of the if where it's used. I remember reading that negations (not) should be avoided when possible, as they're slightly hard to comprehend. Even if you did want to have the if bodies in that particular order, you could use (if-not (empty? coll)...) instead.

Nice idiomatic naming though. Your use of dash-naming and ? suffixes are completely appropriate here and idiomatic.


(def input-lines (line-seq (clojure.java.io/reader "input.in")))

Nothing overly wrong here. Of course, hard coding the file path right inside the def isn't necessarily ideal, but for a toy program like this, it's understandable.

This makes your program impossible to test on my end though, as I have no clue what data you're using. I'm unable to run your program, so I'll simply be "translating" between concepts that should be equivalent.


Your IIFE (Immediately invoked function expression; a term from Javascript) is a little odd here. I can't say I've ever seen code like this. This has a few downsides:

  • Lisp writers can get a little off-put by seeing ((. Often, two opening brackets beside each other either means some fancy is going on, or you have a bug waiting to be run. In this case, it's the former, but it doesn't look ideal.

  • This entire chunk of code has no name associated with it at all! You have the entire main part of your program top-level as a self-calling anonymous function call. This gives no hints as to what this chunk of code is doing

  • Having top level expressions that do a lot of work is a problem when developing with a REPL. When I load this into IntelliJ, every time I reload the file, the whole thing runs! Ideally, you should be manually invoking code from the file instead of having the code in the file run when the file is simply loaded.

I would, at the very least, change this to:

(defn main-routine [ids seen-patterns]
  (when (empty? ids)
    (throw (Exception. "Didn't find match")))

  (let [[next-id & other-ids] ids
        match-patterns (get-match-patterns next-id)
        matches (s/intersection match-patterns seen-patterns)]
    (if (not-empty? matches)
      (prn (apply str (filter #(not= \* %) (first matches))))
      (recur other-ids (apply conj seen-patterns match-patterns)))))

(defn main []
  (main-routine input-lines #{}))

Now you have a little more control regarding when your program actually executes.


While straight recursion is usually fine, Clojure has loop that makes ""recursion"" simpler in some cases. Your code looks like it could be rewritten as:

(defn main-routine [ids seen-patterns]
  (loop [[next-id & other-ids] ids]
    (if next-id ; next-id will be nil if ids is empty
      (let [match-patterns (get-match-patterns next-id)
            matches (s/intersection match-patterns seen-patterns)]
        (if (not-empty? matches)
          (prn (apply str (filter #(not= \* %) (first matches))))
          (recur other-ids (apply conj seen-patterns match-patterns))))

      ; This should be a more specific exception.
      ; It also might just be appropriate to return nil, indicating failure.
      (throw (Exception. "Didn't find match")))))

The gain in this scenario isn't huge though, if there's any at all. I just wanted to point out the existence of loop since I often see it ignored, even though it's exceedingly helpful when doing complicated, multi-accumulator reductions.


This is very good code though, especially for someone new to the language. Except for your lack of ns (and even that isn't necessary here), everything I saw is mostly nitpicking. I look forward to seeing more review requests from you.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.