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 if
s
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 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."))
vs.
(printf "%s hits %s for %d. %s has %d left.\n" aname dname aswing dname dhp)