Clojure function composition: logging results of an action performed by a "pure" function

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))
hp))

(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 ( thechars
(assoc-in [d :alive?] false)))
(do
(logfight (:name attacker) (:name defender) lastswing hp)
(-> thechars
(assoc-in [d :hp] result)
(assoc-in [a :las

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:

-
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.

-
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)]
    (cond
      (not alive?) 
      (printf "%s is already dead!\n" (:name @defender))
      
      (<= result 0)
      (do 
        (printf "%s died!\n" (:name @defender))
        (swap! defender assoc :alive? false))

      :else
      (do
        (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

StackExchange Code Review Q#49371, answer score: 2