4
\$\begingroup\$

I have recently started learning Clojure, and decided to write a number-guessing-game as an exercise.

The game works as follows:

  1. User enters minimum and maximum values
  2. Computer generates a number between those two values (inclusive)
  3. User has to guess the number (the computer always tells him, whether his guess is bigger or smaller then the secret number)

(ns numguess.core)


(defn parse-int [v] 
   (try 
     (Integer/parseInt (re-find #"^-?\d+$" (.toString v))) 
     (catch NumberFormatException e nil)
     (catch NullPointerException e nil)))


(defn get-input
  ([] (get-input nil))
  ([default]
     (let [input (clojure.string/trim (read-line))]
       (if (empty? input)
         default
         (clojure.string/lower-case input)))))


(defn get-number-input
  []
  (let [n (parse-int (get-input))]
      (if (nil? n) (recur) n)))


(defn get-input-prompt
  ([prompt default] (println (str prompt " ")) (get-input default))
  ([prompt] (get-input-prompt prompt nil)))

(defn get-number-input-prompt
  [prompt] 
  (println (str prompt " ")) (get-number-input))


(defn gameplay
  [my-num]
  (let [n (get-number-input-prompt "What is your guess?")]
    (if (< n my-num) 
        (do (println "My Number is bigger") 
              (recur my-num))
        (if (> n my-num) 
            (do (println "My Number is smaller")
                 (recur my-num))
            (println "You have guessed it: congratulations :)")))))


(defn is-yes
  [prompt]
  (let [answer (get-input-prompt prompt "y")]
       (not= "n" answer)))


(defn generate-my-number
  [min-val max-val]
  (let [my-max (max min-val max-val) my-min (min min-val max-val)]
  (+ (rand-int (- (inc my-max) my-min)) my-min)))


(defn mainloop []
  (let [n (generate-my-number 
            (get-number-input-prompt "Please enter minimal value")
            (get-number-input-prompt "Please enter maximal value"))]
    (gameplay n)
    (if (is-yes "Would you like to play again?") (recur))))

(mainloop)

I am interested in any kind of improvement suggestion, no matter whether it is something as important as a key concept, or just indentation.

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

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:

  1. Prompt the user for input
  2. 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))))
\$\endgroup\$
  • \$\begingroup\$ Many thanks for your helpful suggestions, I really appreciate them. I have just one concern: making bigger functions, instead of smaller ones, indeed improves readability, however I am afraid that it could make unit testing more difficult. I would be really interested on your thoughts on this issue. Also, I would kindly ask you to post your version if possible (in particular, I could not find a way to merge gameplay and generate-my-number into mainloop, withouth making it too complex). Btw, I created an improved version, based on your comments. \$\endgroup\$ – Attilio Aug 6 '15 at 20:39
  • \$\begingroup\$ @Attilio See my edits. Please let me know if you have any more questions. I'd be happy to answer :) \$\endgroup\$ – Tim Pote Aug 7 '15 at 0:17
  • \$\begingroup\$ Thank you for the update, I like the conciseness of your solution, besides, I find prompt-int and prompt-bool very elegant :) \$\endgroup\$ – Attilio Aug 7 '15 at 20:03

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.