Rock-Paper-Scissors game in Clojure

Question

I followed braveclojure book and built this little command line Rock, Paper, Scissors game. The game works fine, but I was wondering if there is a better / more elegant / more clojure-y way to deal with side effects?

For example, the body of my play-round function is just a bunch of printlns and it doesn't return anything (well, nil by default).

And I tried to keep such functions to a minimum, but still...

Or perhaps I'm being paranoid, because after all, an application without side - effects is useless.

(ns rps.core
  (:gen-class))

(defn get-input
  "Waits for user to enter text and hit enter, then cleans the input"
  ([] (get-input ""))
  ([default]
   (let [input (clojure.string/trim (read-line))]
     (if (empty? input) default input))))

(defn get-random-choice
  "Let the computer pick a random choice"
  [choices]
  (-> choices keys rand-nth))

(defn update-player-choice
  "Add r / p / s to the list of choices"
  [players player choice]
  (update-in players [player :choices] conj choice))

(defn get-round-winner
  "This function returns a keyword of the
   winning player or nil if it is a draw"
  [user-choice computer-choice]
  (cond
    (= user-choice computer-choice) nil
    (or (and (= user-choice :r) (= computer-choice :s))
        (and (= user-choice :p) (= computer-choice :r))
        (and (= user-choice :s) (= computer-choice :p))) :user
    :else :computer))

(defn increment-player-score
  "Increment the winner's score"
  [players winner]
  (update-in players [winner :score] inc))

(defn update-player-scores
  "If there is a winner, update the winner's score
  otherwise return the original state of players"
  [players winner]
  (if (not (nil? winner))
    (increment-player-score players winner)
    players))

(defn get-round-winner-name
  "Display the name of the round winner"
  [players winner]
  (get-in players [winner :name]))

(defn game-is-on
  "Determine if the game is still on by
  checking that both scores are < 3"
  [players]
  (every? #(-> % :score (< 3)) (vals players)))

(defn generate-players
  "Return a simple object of players in the game"
  ([user-name] (generate-players user-name "Computer"))
  ([user-name computer-name]
   {:user {:score 0
           :choices []
           :name user-name}
    :computer {:score 0
               :choices []
               :name computer-name}}))

(defn display-scores
  "Display the scores and end the game"
  [players]
  (let [user-score (get-in players [:user :score])
        comp-score (get-in players [:computer :score])
        user-won? (> user-score comp-score)
        user-name (get-in players [:user :name])
        comp-name (get-in players [:computer :name])]
    (println (format "%s won the game with the score of %s to %s"
                     (if user-won? user-name comp-name)
                     (if user-won? user-score comp-score)
                     (if user-won? comp-score user-score)))))

(defn display-round-intro
  "A helper function that displays i.e. Rock vs Scissors"
  [choices user-choice computer-choice]
  (println (format "%s vs %s" (get choices user-choice) (get choices computer-choice))))

(defn display-question
  "Display the key question - Rock, Paper, Scissors?"
  [choices]
  (let [question (->> choices
                      (map #(format "%s(%s)" (second %) (-> (first %) name)))
                      (interpose ", ")
                      (apply str))]
    (println (str question "?"))))

(defn play-round
  "The core game logic"
  [players choices]
  (display-question choices)
  (let [user-choice (-> (get-input) keyword)
        computer-choice (get-random-choice choices)
        round-winner (get-round-winner user-choice computer-choice)
        updated-players (-> players
                            (update-player-choice :user user-choice)
                            (update-player-choice :computer computer-choice)
                            (update-player-scores round-winner))]

    (display-round-intro choices user-choice computer-choice)
    (if (nil? round-winner)
      (println "Draw")
      (println (format "%s has won the round" (get-round-winner-name players round-winner))))

    (if (game-is-on updated-players)
      (play-round updated-players choices)
      (display-scores updated-players))))

(defn ask-for-name
  "Get the name from the user"
  []
  (println "What is your name?")
  (let [user-name (get-input)
        players   (generate-players user-name)
        choices   {:r "Rock"
                   :p "Paper"
                   :s "Scissors"}]
    (play-round players choices)))

(defn -main
  "Start the game"
  [& args]
  (println "Let the games begin")
  (ask-for-name))

Answer 1

This is fairly good code. I don't really have much bad to say about it. Most of my suggestions will be stylistic, or based on little things I've learned that have helped me.

First though:

I was wondering if there is a better / more elegant / more clojure-y way to deal with side effects?

Really, you only have a few impure aspects of your program:

• get-input: You need to get input from the user, and have sectioned the functionality off into a single function that you use everywhere. That's pretty much the best you can do.

• The display- functions: These functions are arguably doing too much. They're compiling the data together into a String, and displaying the String. What if you made this program networked in the future, and wanted to use the same functionality? I'd prefer to create format- functions, and println the returns from them. You could also pass in a Stream (like *out* to print to the stdout), and print into the Stream. That way, the user can use a StringStream if they just want the formatted String.

• ... and consequently play-round: Of course, where you tie everything together, you're going to have some side effects. Even the strictest Haskell programs have to have a procedure somewhere. The idea is to create pure functions wherever possible, and section the side-effect causing functions off and test them separately. Here, it could be argued that using println is forcing the user to only print to the console, but you'd likely need to rewrite the procedure for another circumstance anyway, so this isn't a huge problem.

---

---

Some more general observations:

You never do any input validation! I got some real funky results by entering nonsense. My personal library function that I use for getting simple console input is:

(defn ask-for-input
  "Prompts the user for input, checks it using the validation function, and displays the error if the validation fails. Newlines aren't added after the messages."
  [prompt-message error-message validate-f]
  (print prompt-message)
  (flush)

  (let [result (read-line)]
    (if (validate-f result)
      result
      (do
        (print error-message)
        (flush)

        (recur prompt-message error-message validate-f)))))

Yes, this has a lot of side effects, but I've found that any time I needed to ask for, and validate user input, this is essentially what I come up with, so I decided to just wrap that common code in a function. I nearly always need a prompt, a way to validate, and to display an error message, so this has proved very helpful. Using this, you could ask for a player's move in a manner more like:

(ask-for-input (format-question choices) ; As mentioned above
               "Invalid choice!"
               choices) ; Maps return nil (falsey) for a invalid key

That way, you know the data you're getting is definitely valid, and you can go ahead and use it.

---

The keywords you're using to represent things can be difficult to find. I've began to explicitly "declare" the keywords that I'm using at the top of my file, like I was declaring an Enum. You have two main "Enums": :user+:computer, and :r+:p+:s. I would explicitly write these at the top of the file, and, I would use namespaced keywords (::) instead:

(def valid-moves #{::rock, ::paper, ::scissors})
(def valid-player-types #{::user, ::computer})

This has multiple benefits:

• You don't need to go searching through the file if you come back to the project later to see what keywords you're using; everything is explicit at the top of the file. If you're using an intelligent IDE, this also gives it a heads up of what you'll be using so it can give completion hints easier.

• By putting them in a global set, you can easily check if a move is valid:

  (valid-moves "Some invalid nonsense") ; nil - Falsely on a bad lookup
  (valid moves ::rock) ; ::rock - A truthy value, so it's valid
  

• By namespacing the keywords and again, using an good IDE, all you have to do is write ::, and it can immediately suggest the correct keywords to use. This is convenient, prevents keyword spelling errors, and helps prevent you from using the wrong keyword that was used elsewhere or a potential previous misspelled keyword.

---

Really, it seems like you should have a Player record:

(defrecord Player [score choices user-name])

and then create a pseudo-constructor to reduce redundancy:

(defn new-player [username]
  (->Player 0 [] username))

Then you can simplify generate-players to basically:

(defn generate-players2 [user-name computer-name]
   {::user (new-player user-name)
    ::computer (new-player computer-name)})

---

Be very careful returning nil from functions. If you forget to handle the nil somewhere, you're likely to get a NullPointerException, which doesn't give very helpful hints about what might have gone wrong. I'd go one of two ways:

• In the event of a tie, return something like ::tie. This at least has a chance of giving you more information down the road in case something goes wrong. You'll at least know where the bad data originated from.

• Return nil, but make it very clear that the function may return nil. Documentation is great, but I've started taking it a step further ending the names of such functions with a ?. This is dancing with Hungarian Notation, but I like the reminder that a function returns nil. Using the function always involves the question of failure, so I think that should be heavily reflected. This also allows you to make use of when-let and if-let. These seem useless when you first encounter them, but they've grown on me. I tried writing your play-round to make use of if-let, but it got quite messy unfortunately. In this specific case, I'd use option 1.

---

play-game is susceptible to a Stack Overflow! You're using recursion without using recur, which is "dangerous". If somehow the players manage to tie over and over again, it will crash. Change the recursive call to:

(if (game-is-on updated-players)
  (recur updated-players choices) ; Here
  (display-scores updated-players))))

---

---

My brains fried from a long day of work, and Edge is started to lag this is getting so long. I'll post back if I think of anything else, but these were the main things I noticed.