A port of the 1971 StarTrek game to Clojure

Question

I ported the Star Trek 1971 game to Clojure to help me learn more about the language and functional programming in general. You can see the entire game on Github. The game plays (you can start it with lein run). Instructions are i game or you can see the first link to get more details on the history or gameplay.

While porting this game, I picked up on some thing very quickly. For example, I am quite pleased with the main game loop in core.clj.

(defn play-game 
  "The main game loop. When you quit out, the game shuts down. Otherwise a new game starts
  immediately after the old one was finished."
  []
  (w/new-game-state game-state)
  (n/enter-quadrant game-state)
  (let [stop-condition (ref false)]
    (while (not (deref stop-condition))
      ; check to see if the Enterprise is destroyed
      (if (game-over? game-state)
        (do
          (w/new-game-state game-state)
          (n/enter-quadrant game-state))
        (do
          (w/short-range-scan-command game-state)

          (println "COMMAND")
          (let [choice (read-line)]
            (condp = choice
              "0" (n/set-course-command game-state)
              "1" (w/short-range-scan-command game-state)
              "2" (w/long-range-scan-command game-state)
              "3" (e/fire-phasers-command game-state)
              "4" (e/fire-torpedoes-command game-state)
              "5" (e/shield-control-command game-state)
              "6" (e/damage-control-report-command game-state)
              "7" (c/computer-command game-state)
              "q" (dosync (alter stop-condition (fn [_] true)))
              (command-help)  

Simple functional expressions to control where user input goes for each option. I like this, very simple, clear and easy to read.

Where I ran into issues, is that I could not achieve the same level of clarity everywhere.

In the nav.clj file, I had many issues attempting to simplify the movement commands. The function below is invoked every time the player enters a new quadrant.

(defn- leave-quadrant 
  [game-state factor coord dir-vec]

  (let [place (warp-travel-distance (get-in @game-state [:enterprise])
                                    factor
                                    dir-vec)
        energy (- (get-in @game-state [:enterprise :energy])
                  (+ -5 (* 8 (int factor))))
        q (vec (->> (map #(int (/ % 8)) place)
                    (map #(max % 1))
                    (map #(min % 8))))
        s (vec (map #(math/round %) (map - place (vec (map #(* 8 %) q)))))]

    (swap! game-state update-in [:enterprise] merge {:sector s :quadrant q :energy energy})

    (when (> factor 1)
      (swap! game-state update-in [:stardate :current] inc))

    (w/update-lrs-cell game-state (get-in @game-state [:quads (u/coord-to-index q)])))

  (swap! game-state assoc-in [:current-klingons] [])
  (enter-quadrant game-state))     

In this method I do several things that seem wrong. I use a chain of lets to create local variables, I update the global state (the game-state atom) several times. This functions reads as a very imperative style and I would like guidance on how to clean this method up.

I also did not find any opportunities to use macros when writing this game. Suggestions for good uses of macros are very welcome.

These two functions in enterprise.clj drive me crazy. Finally understanding the thrush operator helped immensely, but I find these methods as needlessly complex. I think there must be an elegant way to clean this up, but I simply do not see how.

(defn- enterprise-attack [game-state power k-count klingon]
  (let [p [(:x klingon) (:y klingon)]
        h (-> @power
              (/ k-count
                 (u/euclidean-distance
                   (get-in @game-state [:enterprise :sector])
                   p))
              (* 2 (r/gen-double)))
        z (max 0.0 (- (:energy klingon) h))]

    (u/message (format "%f UNIT HIT ON KLINGON AT SECTOR %s\n   (%f LEFT)"
                     h
                     (u/point-2-str p)
                     z))
    (assoc klingon :energy z)))

(defn- fire-phasers [game-state]
  (when (pos? (get-in @game-state [:enterprise :damage :computer_display]))
    (u/message " COMPUTER FAILURE HAMPERS ACCURACY"))

  (let [power (atom (select-phaser-power (get-in @game-state [:enterprise :energy])))
        k-count (count (get-in @game-state [:current-klingons]))]
    (swap! game-state update-in [:enterprise :energy] - @power)
    (swap! game-state assoc-in [:enterprise] (k/klingon-turn 
                                               (get-in @game-state [:enterprise])
                                               (get-in @game-state [:current-klingons])))

    (when-not (neg? (get-in @game-state [:enterprise :shields]))
      (when (neg? (get-in @game-state [:enterprise :damage :computer_display]))
        (swap! power #(* % (r/gen-double))))

      (swap! game-state assoc-in [:current-klingons] 
             (->> (get-in @game-state [:current-klingons])
                  (map #(enterprise-attack game-state power k-count %))
                  (map #(if (pos? (:energy %)) % (phasers-hit-klingon game-state %)))
                  (remove nil?)
                  (vec))))))

When reading these two methods I feel overwhelmed by the large number of swaps. I also hate that I needed to make power an atom so I could change the efficiency of the phasers (by reducing power) when certain systems are damaged.

Thanks for the time looking at my first real stab at writing something non-trivial in Clojure. Any feedback welcome. I would really love for anyone to checkout the overall architecture and flow of the game and offer suggestions for improvement.

Answer 1

This is a really cool project, and I don't think the non-functional aspects are as problematic as they might seem. One of the things that makes Clojure so flexible is that it lets you be imperative, or even object-oriented, when it makes sense. I think most of the messiness in this code comes from insufficient generalization, rather than being imperative. If you do want to make it more purely functional, James Hague has written some really interesting essays about game programming in functional languages (he uses Erlang, which is even more purely functional than Clojure); the most famous is Purely Functional Retrogames, where he takes apart the process of implementing Pac-Man in Erlang in a purely functional manner, without passing around a "game state" variable. He also argues in Functional Programming Doesn't Work (and what to do about it) that even in a purely functional language, there are certain situations where an imperative "pressure relief valve" is extremely useful and that we shouldn't contort ourselves trying to avoid those. We Clojure users have it good here, because Clojure has high-quality imperative pressure relief valves like the reference types.

I've never written a game in Clojure, but I'll try to offer some suggestions based on general principles and my understanding of Hague's advice, starting at a low level and moving up.

In general, I think you could clean up your code quite a bit if you had more small helper functions. For example, I would put the calculation of h in enterprise-attack into another function:

(defn- hit-value [pos1 pos2 power k-count]
  (-> power
    (/ k-count (u/euclidean-distance pos1 pos2))
    (* 2 (r/gen-double)))

Even if this function is only used once, the code inside is ugly enough that the call site will look nicer without it. The same is true for a lot of the calculations in the main let of leave-quadrant.

You asked where macros might have been helpful, and I see one obvious place. You have the code (get-in @game-state [:something :something-else]) and (update-in @game-state [:something :something-else] some-fn args) all over the place. You could write a helper function or macro to shorten that up, maybe something like this (as a function):

(defn get-state [& keys]
    (get-in @game-state keys))

(defn update-state [keys f & args]
    (apply (partial update-in @game-state keys f) args))

Then you can call (get-state :enterprise :energy) instead of (get-in @game-state [:enterprise :energy]) and (reset! game-state (update-state [:enterprise :energy] - @power)) instead of (swap! game-state update-in [:enterprise :energy] - @power) You could also put a swap! inside update-state to make things even shorter.

These changes give you a kind of API to shorten and simplify interaction with your game state.

Here's what those helpers would look like as macros. (Hopefully; I'm far from a macro expert.)

In this case, since we're not doing anything special with the order of evaluation, the macro version of get-state looks exactly the same as the function version.

(defmacro get-state [& keys]
    (get-in @game-state keys))

(defmacro update-state [keys f & args]
    `(update-in @game-state ~keys ~f ~@args))

You could also throw a swap! inside the macro version of update-state, like this:

(defmacro update-state [keys f & args]
     `(swap! game-state update-in ~keys ~f ~@args))

Then you could do destructive updates to the game state much more succinctly.

On the other hand, having a giant, monolithic game-state variable being passed through every function is sort of not functional to begin with. In the following code, I'm going to treat game-state as a global, because if you have a mutable variable that you always, without fail, pass into every function, wherein you make destructive updates to it, you basically have a global. Let's cut the clutter and just treat it as such. James Hague discusses this issue in Purely Functional Retrogames Part 3 and Part 4, and I'll go over what he says below, but for now, we'll have a global game state, because I do prefer that having a global game state that takes up space in the argument list of every function.

To get out of doing destructive updates, Hague suggests unpacking only the pieces of the game state which are relevant to a given function, passing them in, and returning a value which represents the effects on the game state to the main loop. The main loop can then update state based on what value it gets back. This would get rid of a lot of the calls to swap! in your functions; you could bundle up all the necessary changes inside the functions, return those changes, and then have a single swap! at a higher level that makes those changes. With this kind of system, you might move between quadrants something like this:

(defn transition-quadrant
  [factor coord dir-vec]
  (swap! game-state merge (leave-quadrant factor coord dir-vec))
  (enter-quadrant))

(defn- leave-quadrant 
  [factor coord dir-vec]
  (let [place (warp-travel-distance (get-state :enterprise)
                                    factor
                                    dir-vec)
        energy (- (get-state :enterprise :energy)
                  (+ -5 (* 8 (int factor))))
        q (vec (->> (map #(int (/ % 8)) place)
                    (map #(max % 1))
                    (map #(min % 8))))
        s (vec (map #(math/round %) (map - place (vec (map #(* 8 %) q)))))]
   {:enterprise {:sector s,
                 :quadrant q,
                 :energy energy}
    :stardate (update-in @game-state [:stardate :current]
                            #(if (> factor 1) (inc %) %))
    ;; Include whatever changes update-lrs-cell does here
    :current-klingons []})    

Now leave-quadrant does not ever update the game state directly; it just returns a map of things which have changed between the previous version of the state and the current one. transition-quadrant merges the new state and the old state, replacing the values which have changed and leaving alone the ones which haven't. This kind of refactoring could also really clean up fire-phasers

Your use of a bunch of chained definitions in a let is something I've seen in lots of other Clojure code, and it is purely functional, so I wouldn't worry about it. When you calculate q from place, you're not doing IO, you're not destructively updating a variable, and you're not breaking referential transparency.

Another thing Hague suggests is keeping around only the minimum amount of state. He advises figuring out which pieces of data depend on other things, and calculating them as needed, rather than storing them. In your code, you could do this with power; instead of an atom that contains the current power, you could make another function, calculate-power. The Enterprise state could contain a base power level that you pass to calculate-power, and then the function can calculate the actual current output, returning a lower number if damage has occurred, and maybe a higher number if you've got some kind of secret Romulan power generator installed. This would also clean up fire-phasers a bit.

Here's a slightly cleaner version of fire-phasers as an example of all the suggestions I've made (except that it still has the global game state):

(defn- fire-phasers [game-state]
  (when (damaged :enterprise :computer_display)
    (u/message " COMPUTER FAILURE HAMPERS ACCURACY"))

  (let [power (calculate-power (get-state :enterprise :energy))
        klingon-count (count (get-state :current-klingons))]
    {:enterprise (merge (k/klingon-turn (get-state :enterprise)
                                        (get-state :current-klingons))
                        {:energy (- (get-state :enterprise :energy) power)})
     ;; Note: the whole (when-not (neg? shields...)) bit has been moved
     ;; to calculate-power.
     :current-klingons (->> (get-state :current-klingons)
                            (keep (partial enterprise-attack power klingon-count))
                            (vec))
     ;; Note: the #(if (pos? (:energy %)) % ...) part has been moved inside
     ;; enterprise-attack.
     }))

I've assumed some parts were moved into helper functions. In particular, I'm envisioning calculate-power as a single function which takes into account everything that affects the power, and the whole part that calculates the effect of the Enterprise's attack on the Klingons has gone into enterprise-attack.

I once again rearranged the code to return, essentially, a diff between the old state and the new state, which can be used to modify the global state at a higher level. I find this a bit cleaner and more purely functional than using swap! directly inside the function.

I just recently discovered the keep function myself. It's exactly the same as map, except it automatically removes nil from the sequence, so you don't have to do (->> coll (map f) (remove nil?)). There's also keep-indexed to replace map-indexed.

At the very highest level, I recommend finding an alternate scheme for the game state. James Hague says "In a functional language, the worst thing you can do is create a large 'struct' containing all the data you think you might need for an entity", and argues why, in Purely Functional Retrogames Part 3. His performance argument is not totally applicable to Clojure, because of structural sharing and transients, which the compiler uses to minimize copying and allow some mutable state. His arguments about flexibility and clarity, though, are worth considering. Using a global game state is inflexible because I have to figure out before I write a single function how I'm going to store that state, since I have to bake how I access it into every function. With your current code, if you decided to change from using an atom to using a ref, or a record, or just a plain map, or a Java class, you'd have to go through and change every single swap! to something else. The state access API I suggested (the get-state and update-state macros) helps this a little, because you can just change those macros. But using a global game state is also unclear, because seeing that a function takes the game state as an argument gives me no insight into what it's actually doing with the state and what parts of the state it really needs. When I see the call (enter-quadrant game-state), I gain no insight from the arguments into what enter-quadrant does, whereas I do get some insight from (enter-quadrant x y) or (enter-quadrant :enterprise).

My first thought for how to represent data would be something with deftype or defrecord. These are lean, vaguely object-oriented structures which can be used much like maps, but can also implement protocols, which are sort of like interfaces in Java. I might have a FederationShip record which holds essential state like energy and damage, and implements a Starship protocol that contains various functions which calculate other pieces of state, like power, the raw damage its weapons can produce given the current state, and whether the ship is capable of going to warp in its current condition. The Enterprise would be a single record of the FederationShip type. I might also have a KlingonShip record that implements the Starship protocol and the Enemy protocol, which would create functions that only enemies need to have. That way, we have functions that take Enterprise or KlingonBirdOfPrey instead of just game-state.