I’ve written my first non-trivial program and am looking for feedback in terms of style, alternative approaches, or any other suggestions for improving it.
I’m hoping the intent is clear from the code / comments. Thanks!
;; pick <limit>
;; display a random integer between 1 and <imit> inclusive, ensuring
;; previously selected values are not repeated, remembering history
;; betwen invocations
;; use first command line param as limit and fail if it isn't present
(let [limit (first *command-line-args*)]
(when-not limit
(println "usage: pick <limit>")
(System/exit 0))
;; if (chosen-<limit>.txt) exists, open it and read history of chosen numbers
(let [file-name (str "chosen-" limit ".txt")
chosen-numbers-str (if (.exists (clojure.java.io/file file-name))
(slurp file-name)
"")
chosen-numbers (set (clojure.string/split chosen-numbers-str #"\s+"))
all-numbers-seq (range 1 (+ (Integer/parseInt limit) 1))
all-numbers (set (map str all-numbers-seq))
available-numbers (clojure.set/difference all-numbers chosen-numbers)]
;; pick number from 1 to <limit> inclusive that has not already been chosen
(when (empty? available-numbers)
(println "sorry, all numbers have been picked")
(System/exit 0))
(let [selected-number (rand-nth (vec available-numbers))]
(println selected-number)
;; create chosen-<limit>.txt if it doesn't exist
;; write out latest choice to chosen-<limit>.txt and close file
(spit file-name (str selected-number "\n") :append true))))
That’s readable code leveraging core libraries; good for an initial effort.
When I went through it, and thought about how I would approach the problem, I defaulted toward a couple of alternative ideas:
1 - leverage the clojure reader and .edn as a simple format for serializing the state.
2 - separate out steps into smaller functions and compose them to get the work done.
1 makes things easy since we can alter the algorithm for generating random picks to not even use sets. Instead, we can just generate a random sequence of numbers once, and store it for draws. When we run out of numbers in the sequence, we’re done. No checking for previously chosen values, no need to manually parse things (the reader does it for us).
2 helps make things a bit more readable (and easy to alter). I was able to extend the api after the fact by threading through an optional argument to reset the sequence of draws, so once the draws are exhausted a new sequence can be initiated. It also allows us to sort of simplify the control flow and factor out the imperative branches (like the multiple calls to System/exit depending on failure conditions). I opted for a simple api to advance the random number sequence and record the choice (using a clojure map). We basically use sequential transformations of the map to communicated stuff by adding/removing keys, and let the absence or presence of data drive the control flow. Clojure also tends to leverage the idea of nil punning, where the absense of a thing (the nil value) conveys meaning too. In this case, if the pick function can’t advance because there aren’t any numbers left, it will return nil, which can be picked up by consumers like draw! to indicate absence. If there aren’t any numbers, we can inform the user.
I also targeted using the clojure cli to run this as a little script:
Thank you so much for your thoughtful response. You’ve given me a lot to go research and think about, particularly in regard to the reader and edn which I’m not familiar with except as buzzwords. I haven’t really had time to digest your code in any detail, but I have a couple of quick comments:
One thing I didn’t like about my code was the “nested lets”. In something like C or C#, checking for early “bail out cases” like I did would avoid the use of nested ifs. It seems that something analogous was happening with my nested lets despite my usage of the “early bail outs”. To some degree you ameliorated this problem just by breaking things up into functions with their own scopes.
You mentioned that you targeted clojure cli to run this as a script. To avoid confusing the issue, I didn’t mention that I’m actually running this under Windows like you, and that I’m using babashka with the recipe described here to create a self executing .bat file to run the code. I’ve never used clojure cli, so I’d be curious how the startup times for small scripts compared to babashka.
In something like C or C#, checking for early “bail out cases” like I did would avoid the use of nested ifs.
You can use let as an imperative construct if you ignore the return or you have bindings that eventually throw an exception or similarly bail out (like System/exit). You can repackage your nested lets into one:
There are also useful macros like some->> and some-> that will pass along and short circuit when they hit a nil return. You can cook up macros to handle custom control flow (e.g. a special version of let that will quit if a binding fails maybe), or use libraries like GitHub - adambard/failjure: Monadic error utilities for general use in Clojure(script) projects for more sophisticated composed error values.
Your picker code is very nice @joinr, thank you for sharing! But I believe running it with clj -M picker.clj 3 will not execute anything unless run is called by something. Adding (run) at the end of the script worked for me.
