Styling
- All forms should be separated by a single newline.
All let bindings should be on their own line. So
(let [my-max (max min-val max-val) my-min (min min-val max-val)] ... )
Should be:
(let [my-max (max min-val max-val)
my-min (min min-val max-val)]
Instead of fully qualifying other namespaces, you should require them in your ns delclaration:
(ns numguess.core
(:require [clojure.string :as string]))
(string/trim ...)
(string/lower-case ...)
Structure
Before I even read the requirements, I could tell this was some sort of number game. So that's good.
However, I found it very difficult to figure out how the user input was being read in. get-input is buried
three layers deep for every codepath it's in. Input is parsed ad-hoc in various places, and occasionally the input is altered by default.
I would suggest trying to clearly separate your concerns here. Your objectives are:
- Prompt the user for input
- Parse that input into usable data (this would include parsing
"" to a default value)
If you can separate those things out, you can easily expand Number 2 for any data type you need to parse (e.g. boolean for the "y/n" prompt).
Also, gameplay can be simplified by using a cond rather than nested ifs.
All of that being said, the structure of mainloop gives a pretty clear overview of the entire program. So bravo there.
General Notes
I've said this in another Code Review answer, but I think it applies here too.
When I moved from Java to Clojure, I tended to like very small functions (e.g. is-yes, get-input-prompt, and get-number-input).
Over time, I've found it's a) more idiomatic and b) easier to understand if you start with a rather large function and break it up as needed to
increase understanding. My reasoning for this preference is that Clojure already gives you really good, simple building blocks.
This allows you to define the simple functions that directly enrich your domain without worrying as much about length.
If I want added documentation, I will let a variable with a good name.
In the solution I wrote while preparing this code review, I didn't even include a gameplay or a generate-my-number function. Those pieces
ended up being small enough to fit in mainloop without making it too complex. And, as I said, if you can do that, it actually ends up being
easier to understand because you're not creating artificial abstractions just to hide complexity.
Update in response to OP's comment
Just to clarify my statement about function size, the goal isn't to have small functions or large functions. The goal is to have functions that do one thing, and do it in a clear, concise way.
That goal is really what programming is all about, no matter the language.
The only difference I've found with Clojure is it's easier to express higher concepts clearly. So you're not looking for arbitrary seams just to make your function smaller. You look for concepts that you can cleanly abstract out.
All that to say, if you've found the right places to break up your program, testing should be easier, not harder. Because you're working with a particular domain concept instead of an arbitrary seam.
Additionally, I find testing less necessary overall. I spend more of my time trying to find those simple concepts and less of it building guard rails for myself. Take, for example target-num in my solution below. I could abstract that out (it is a single concept, so it wouldn't be a bad idea) and write a test for it, but a) it's random, so bleh, and b) I can see everything it's doing right there. So there is no real benefit to writing a test around it.
YMMV on the testing thing. Many people find it valuable as a feedback mechanism rather than a guardrail. Just my personal take. :)
Here's my solution:
(defn prompt [parser prompt-text]
(println prompt-text)
(if-some [value (try
(parser
(string/trim
(read-line)))
(catch Throwable _))]
value
(do
(println "I'm sorry. I didn't understand you.")
(recur parser prompt-text))))
(defn parse-int [s]
(Integer/parseInt s))
(defn parse-bool [s]
({"y" true
"yes" true
"true" true
"n" false
"no" false
"false" false} (string/lower-case s)))
(def prompt-int (partial prompt parse-int))
(def prompt-bool (partial prompt parse-bool))
(defn my-mainloop []
(let [min-val (prompt-int "Please enter minimal value")
max-val (prompt-int "Please enter maximal value")
target-num (+ (rand-int
(- (inc max-val) min-val))
min-val)]
(loop []
(let [guess (prompt-int "What is your guess?")]
(cond
(< guess target-num) (do
(println "My Number is bigger")
(recur))
(> guess target-num) (do
(println "My Number is smaller")
(recur))
:else
(println "You have guessed it: congratulations :)"))))
(when (prompt-bool "Would you like to play again?")
(recur))))
