I'm wondering how I could compose this program better allow it to grow more functionality without constant refactoring.

This is my code sample for Hacker School, so I'm less married to the results but instead am looking to further my understanding of Clojure and functional programming.

Basically, my battle simulator needs to update the state of the battle participants, while also making those actions known to the console where I represent the state of the battle. Every time I add a feature, I seem to be rewriting everything, so I know I'm structuring this wrong.

How would you arrange these functions (specifically, logfight and battleturn to work well together without being intrinsically tied? Right now I carry around the pertinent info (:lastswing) in the atom, but that feels wrong.

Feel free to mention anything else that bugs you in the code.

(ns rpg)

(def characters (atom
                 {:player {:name "Fred" :hp 100 :att 5 :dex 10 :alive? true :lastswing 0}
                  :creature1 {:name "Badman" :hp 99 :att 4 :dex 7 :alive? true :lastswing 0}}))

(defn hit? [dex]
  (> 5 (* dex (rand 1))))

(defn swing [att]
  (int (* att (+ 1 (rand 1)))))

(defn damage [hp swing]
  (- hp swing))

(defn attack [att dex hp]
  (if (hit? dex)
    (damage hp (swing att))

(defn logfight [aname dname aswing dhp]
  (if (> aswing 0)
    (println (str aname " hits " dname " for " aswing ". " dname " has " dhp " hp left."))
    (println (str aname " missed!"))))

(defn battleturn [thechars a d]
  (let [attacker (a thechars)
        defender (d thechars)
        att (:att attacker)
        dex (:dex attacker)
        hp (:hp defender)
        result (attack att dex hp)
        lastswing (- hp result)]
    (if (:alive? defender)
      (if (<= result 0)
           (println (str (:name defender) " died!"))
           (-> thechars
           (assoc-in [d :alive?] false)))
           (logfight (:name attacker) (:name defender) lastswing hp)
           (-> thechars
           (assoc-in [d :hp] result)
           (assoc-in [a :lastswing] lastswing))))
        (println (str (:name defender) " is already dead!"))

(defn fight [target]
  ((swap! characters battleturn :player target)
   (swap! characters battleturn target :player)))

;; The UI
(fight :creature1)

2 Answers 2


Let the side effects happen and return nil

The way you have your fight function has the potential to throw an exception when you try to use it. Notice how you start with two opening parentheses, (( -- this should stick out like a sore thumb to any Lisper, and it's especially rare in Clojure. Keep in mind that because Clojure is a Lisp, it attempts to call the first thing in any S-expression as a function. In this case, (swap! characters battleturn :player target) is the first thing in the single S-expression that constitutes the body of the fight function. That means that whatever that expression evaluates to, Clojure will try to call it as a function, using whatever follows (in this case, (swap! characters battleturn target :player)) as an argument. You don't really want that, though. You just want the side effects from using swap! twice, once for :player target and once for target :player, and then you don't care what it returns. There are a handful of special forms in Clojure that have what's called "implicit do syntax," which means you can have more than one S-expression in the body. defn is one of those forms, so all you have to do to avoid throwing an exception is take out a set of parentheses:

(defn fight [target]
  (swap! characters battleturn :player target)
  (swap! characters battleturn target :player))

Incidentally, this will also greatly simplify the way your battleturn function is structured, because you don't have to worry about returning thechars at the end of every conditional branch. You're working with a mutable collection (the atom characters), so the key thing to remember is that it doesn't matter what you're returning. This is backwards from the way things usually work in Clojure with its functional programming paradigm, but that's the whole point -- your program is utilizing a more object-oriented style, so it's all about mutating values rather than returning something. In this case, you're mutating things like the characters' HP.

Maybe each character should be an atom (also, destructuring)

On that note, I think it would make more sense to represent each character as its own, separate atom, since the character itself is what you are modifying. Maybe try something like this:

(def player     (atom {:name "Fred" :hp 100 :att 5 :dex 10 :alive? true :last-swing 0}))
(def creature-1 (atom {:name "Badman" :hp 99 :att 4 :dex 7 :alive? true :last-swing 0}))

(By the way, this is just a nitpicky thing, but the idiomatic thing to do in Clojure is to hyphenate multi-word symbol names, e.g. battle-turn instead of battleturn, etc.)

Then adjust your functions so that they are referring to the player and the creature directly by their symbols, rather than reaching into characters to get them. This also means you can take thechars out of your battleturn function entirely, which simplifies things:

(defn battle-turn [attacker defender]
  (let [{:keys [att dex]}   @attacker
        {:keys [hp alive?]} @defender
        result              (attack att dex hp)
        last-swing          (- hp result)]

I'll pause here and point out that I shortened your let statement in a couple of ways:

  1. You no longer need to bind attacker and defender to (a thechars) and (d thechars), since we got rid of the characters collection altogether and we're just passing in the atoms for each character to the function explicitly. Notice that we now have to dereference attacker and defender using @, as they are atoms.

  2. I used destructuring to simplify the process of grabbing fields from attacker and defender. Basically what we're saying is "bind the values for :att and :dex from @attacker to att and dex", and the same for :hp and :alive? from @defender.

Prefer cond over nested ifs

Another key point is that it's generally better to use a cond statement than nested if statements when you have several branches of conditional logic like you have here. It can really make your code easier to read if you structure it right. Consider this:

(defn battle-turn [attacker defender]
  (let [{:keys [att dex]}   @attacker
        {:keys [hp alive?]} @defender
        result              (attack att dex hp)
        last-swing          (- hp result)]
      (not alive?) 
      (printf "%s is already dead!\n" (:name @defender))
      (<= result 0)
        (printf "%s died!\n" (:name @defender))
        (swap! defender assoc :alive? false))

        (log-fight (:name @attacker) (:name @defender) last-swing hp)
        (swap! defender assoc :hp result)
        (swap! attacker assoc :last-swing last-swing)))))

The cond statement here makes it clearer that there are only 3 possible outcomes, and in what order they are considered.

printf/format is awesome

You may have noticed that I replaced your use of (println (str ...)) with (printf ...). printf is just a shortcut for (print (format ..., so we're really talking about str vs. format. To be fair, there's nothing wrong with using str, and in fact, it's often shorter than its format equivalent, particularly when you're not dropping a whole lot of things into the string. But I still prefer format, because I think it's a lot more readable, and when you use printf, it's often shorter than the equivalent (print (str ... expression. I don't think there's an "idiomatic" preference of one vs. the other, but I just thought I'd throw that out there for your consideration. Consider:

(println (str aname " hits " dname " for " aswing ". " dname " has " dhp " hp left."))


(printf "%s hits %s for %d. %s has %d left.\n" aname dname aswing dname dhp)
  • You are passing pre-hit hp, instead of the result of the attack to logfight, but it writes " has " dhp " hp left."; that is logged HPs lag behind the actual HPs.
  • Right now I carry around the pertinent info (:lastswing) in the atom, but that feels wrong.

    You're passing the lastswing from the let bindings to logfight, and I can't see where you are using :lastswing attribute, why not just drop it?

  • With multiple swaps, if the first swap throws an exception the second one is not called. Which may leave the game in an inconsistent state. If there are multiple threads similar inconsistencies could occur. Even in the current single-threaded form notice that splitting the state change, that is making the change to the atom non-atomic, may cause inconsistencies; for example currently player may be killed by the creature after he killed it.

  • More importantly, because swap may call the updating function multiple times the function it's called with should not have any side effects(such as print... or rand). Currently if other threads keep changing the state of the atom, you can have Fred still alive after it was announced that "Fred died!" etc.

What you should do is beyond my ken, though, so I won't give any concrete suggestions.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.