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.