Ytdyklly

I'm visiting a functional programming course at my university which has a small project for examination. The language we are using is clojure and the contents of the lecture have mostly been about it's syntax, rather than the benefits and paradigms of functional programming. Our lecturer doesn't seem to be too harsh when it comes to grading but we are still very unsure if our program fully matches the requirements of functional programming. Here are some of our questions:

1. Can you mess up that badly when it comes to functional programming or are some things already ensured when using clojure?


2. We have some functions with random numbers to randomize the output (e.g. "create-star"). This makes them "unpure" because output with the same input is different every time. If we would pass the random numbers via the parameters instead it would make the code much less readable and clear. Is sticking to the parameters of functional programming more important than visibility? (If we would follow that train of thought we would even have to create and pass all random values in the "update-state" functions parameters as it is the outer-most function to be called and would be unpure aswell if it contained randomness.)
   


3. The framework internally uses an atom for the state variable which we modify in some methods. But we always pass it as parameter and return it, so we don't use global variables. Does this violate the principle of immutable objects?


4. Is there anything else in our code below that violates functional programming principles?

Below is the main part of our code. The full project can be found at https://github.com/Niggls/rocket_game. It's a small game where you have to control a rocket to avoid incoming meteors. We are using the "quil" library to draw the contents of the game screen. It works with a "state" variable that handles all data of objects on the screen. The main parts are the "draw" function which creates the output and the "update-state" function which evaluates all of our functions for every frame. Both of these are at the bottom.

(ns rocket-game.core

  (:require [quil.core :as q]

            [quil.middleware :as m]))





(defn star-color-index [x]

  (if (< x 12)

      0

      (if (< x 15)

          2

          3)))



(defn create-star [y]

 {:x (rand-int (q/width))

   :y (rand-int y)

   :size (+ (rand-int 5) 1)

   :speed (+ (rand-int 3) 1)

   :color (star-color-index (rand-int 20))})



;; function for reset state

(defn reset-state-variable [state]

 {:rocket {:image (q/load-image "images/rocket.png")

           :x 260

           :y 340

           :dir 0}

  :background (q/load-image "images/stars.jpg")

  :fires 

  :smoke 

  :score 0

  :stars (:stars state)

  :highscore (if ( > (:score state) (:highscore state))

                 (:score state)

                 (:highscore state))

  :gameOver true

  :meteors 

  :bonus })



;; setup: here we define our global state variable

;; # --> anonymous function

(defn setup 

  ;; these two lines, a map (data structure) is added in step 1-6

  {:rocket {:image (q/load-image "images/rocket.png")

            :x 260

            :y 340

            :dir -1}

   :background (q/load-image "images/stars.jpg")

   :fires 

   :score 0

   :smoke 

   :highscore 0

   :stars (take 150 (repeatedly #(create-star (q/height))))

   :gameOver false

   :meteors 

   :bonus })



;;;; helper methods;;;;;;;;;;;;;;;;;;;;;;;;

(defn inside? [x y]

  (or

   (< x -12)

   (> (+ x 33) (q/width))

   (< y 0)

   (> (+ y 40) (q/height))))



(defn item-inside? [item]

 (let [x (:x item)

       y (:y item)]

  (> y (q/height))))



(defn remove-stars [stars]

 (remove item-inside? stars))



(defn meteor-out [state]

  (let [old (-> state :meteors (count))

        new-meteor (remove item-inside? (:meteors state))

        new (count new-meteor)]

   {:rocket (:rocket state)

    :background (q/load-image "images/stars.jpg")

    :fires (:fires state)

    :score (+ (:score state) (- old new))

    :highscore (:highscore state)

    :gameOver false

    :smoke (:smoke state)

    :stars (:stars state)

    :meteors new-meteor

    :bonus (:bonus state)}))



(defn meteor-hit [state]

 (let [rocket-x (-> state :rocket :x)

       rocket-y (-> state :rocket :y)

       meteors (:meteors state)]

   (if (empty? meteors)

       state

       (if (loop [[m1 & rest] meteors]

             (if (or (and

                       (<= (:x m1) rocket-x (+ (:x m1) 45))

                       (<= (:y m1) rocket-y (+ (:y m1) 45)))

                     (and

                       (<= (:x m1) (+ rocket-x 45) (+ (:x m1) 45))

                       (<= (:y m1) (+ rocket-y 45) (+ (:y m1) 45))))

               true

               (if (empty? rest)

                   false

                   (recur rest))))

           (reset-state-variable state)

           state))))



(defn bonus-out [state]

  (if (item-inside? (:bonus state))

    state

    {:rocket (:rocket state)

     :background (q/load-image "images/stars.jpg")

     :fires (:fires state)

     :score (:score state)

     :highscore (:highscore state)

     :gameOver false

     :smoke (:smoke state)

     :stars (:stars state)

     :meteors (:meteors state)

     :bonus }))



(defn bonus-hit [state]

 (let [rocket-x (-> state :rocket :x)

       rocket-y (-> state :rocket :y)

       bonus (get (:bonus state) 0)]

   (if (empty? bonus)

       state

       (if (or (and

                 (<= (:x bonus) rocket-x (+ (:x bonus) 40))

                 (<= (:y bonus) rocket-y (+ (:y bonus) 40)))

               (and

                 (<= (:x bonus) rocket-x (+ (:x bonus) 40))

                 (<= (:y bonus) (+ rocket-y 45) (+ (:y bonus) 40)))

               (and

                 (<= (:x bonus) (+ rocket-x 45) (+ (:x bonus) 40))

                 (<= (:y bonus) rocket-y (+ (:y bonus) 40)))

               (and

                 (<= (:x bonus) (+ rocket-x 45) (+ (:x bonus) 40))

                 (<= (:y bonus) (+ rocket-y 45) (+ (:y bonus) 40))))

         {:rocket (:rocket state)

          :background (q/load-image "images/stars.jpg")

          :fires (:fires state)

          :score (+ (:score state) (:points bonus))

          :highscore (:highscore state)

          :gameOver false

          :smoke (:smoke state)

          :stars (:stars state)

          :meteors (:meteors state)

          :bonus }

         state))))



;; # defines a function --> (fn [oldAge] (+ oldAge 0.3))

(defn age-smoke [smoke]

  (update-in smoke [:age] #(+ % 0.3)))



(defn old? [smoke]

  (< 2.0 (:age smoke)))



(defn remove-old-smokes [smokes]

  (remove old? smokes))



;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;





;; creation methods ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(defn create-meteor [state]

 (if (= (rand-int 10) 1)

     (if-not (or

               (-> state :rocket :dir (= 0))

               (-> state :rocket :dir (= -1)))

             (update-in state [:meteors] conj {:x (rand-int (+ (q/width) -40)) :y -40 :speed (+ (rand-int 30) 5)})

             ;(update-in state [:meteors] conj {:x (rand-int (+ (q/width) -40)) :y -40 :speed 1})

             state)

     state))



(defn create-smoke [x y]

  {:pos [(+ x 25 (- (rand-int 10) 5))

         (+ y 50 (- (rand-int 10) 5))]

   :dir 0.0

   :age 0.0

   :col [(+ (rand-int 105) 150)

         (+ (rand-int 100) 100)

         (rand-int 100)]})



(defn emit-smoke [state]

  (let [x (-> state :rocket :x)

        y (-> state :rocket :y)]

    (update-in state [:smoke] conj (create-smoke x y))))



(defn create-new-star [state]

 (if(= (rand-int 7) 1)

   (if-not (or

             (-> state :rocket :dir (= 0))

             (-> state :rocket :dir (= -1)))

           {:rocket (:rocket state)

            :background (q/load-image "images/stars.jpg")

            :fires (:fires state)

            :score (:score state)

            :highscore (:highscore state)

            :gameOver true

            :smoke (:smoke state)

            :stars (conj (:stars state) (create-star 1))

            :meteors (:meteors state)

            :bonus (:bonus state)}

           state)

   state))



(defn create-bonus [state]

 (if (and (empty? (:bonus state)) (= (rand-int 100) 1))

   (if-not (or

             (-> state :rocket :dir (= 0))

             (-> state :rocket :dir (= -1)))

           (if (= (rand-int 5) 1)

             (update-in state [:bonus] conj {:x (rand-int (+ (q/width) -40)) :y (rand-int (+ (q/height) -40)) :points 25 :speed 3 :image "images/bonus2.png"})

             (update-in state [:bonus] conj {:x (rand-int (+ (q/width) -40)) :y (rand-int (+ (q/height) -40)) :points 10 :speed 2 :image "images/bonus.png"}))

     state)

  state))



(defn fly-backwards [smoke state]

 (if (-> state :rocket :dir (= 2))

     

     smoke))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;



;;;;;;;;;;;; reset methods;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;



(defn reset-game [state]

  (if

    (inside? (:x (:rocket state )) (:y (:rocket state)))

    (reset-state-variable state)

    state))



(defn reset-game-over [gameOver state]

  (if (-> state :rocket :dir (not= 0))

    false

    true))



;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;,



;;;;;;;; move methods;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(defn move [state event]

  (case (:key event)

    (:w :up) (assoc-in state [:rocket :dir] 1)

    (:s :down) (assoc-in state [:rocket :dir] 2)

    (:a :left) (assoc-in state [:rocket :dir] 3)

    (:d :right) (assoc-in state [:rocket :dir] 4)

    state))



(defn move-meteors [meteor]

 (let [speed (:speed meteor)]

  (update-in meteor [:y] #(+ % speed))))



(defn move-star [star]

 (update-in star [:y] #(+ % (:speed star))))



(defn move-stars [state]

 (if-not (or

           (= (:dir (:rocket state)) 0)

           (= (:dir (:rocket state)) -1))

         {:rocket (:rocket state)

          :background (q/load-image "images/stars.jpg")

          :fires (:fires state)

          :score (:score state)

          :highscore (:highscore state)

          :gameOver true

          :smoke (:smoke state)

          :stars (doall (map move-star (:stars state)))

          :meteors (:meteors state)

          :bonus (:bonus state)}

         state))



(defn move-bonus [state]

  (if (empty? (:bonus state))

    state

    (update-in (:bonus state) [:y] + 4)))



(defn move-rocket [rocket]

 (case (:dir rocket)

  (1) (update-in  rocket [:y] - 10)

  (2) (update-in  rocket [:y] + 10)

  (3) (update-in  rocket [:x] - 10)

  (4) (update-in  rocket [:x] + 10)

  (0) (update-in  rocket [:x] + 0)

  (-1) (update-in  rocket [:x] + 0)))



;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;







; draw method

(defn draw [state]

  ;; q/background-image and q/image functions are added in step 1-6

  ;(q/background-image (:background state))

  (q/background 0)

  (q/fill 250 250 250)

  (q/stroke 250 250 250)

  (doseq [star (:stars state)]

   (if (= (:color star) 0)

       (do

         (q/fill 250 250 250)

         (q/stroke 250 250 250)

         (q/ellipse (:x star) (:y star) (:size star) (:size star)))

       (if (= (:color star) 1)

           (do

             (q/fill 255 255 26)

             (q/stroke 255 255 26)

             (q/ellipse (:x star) (:y star) (:size star) (:size star)))

           (do

             (q/fill 255 77 77)

             (q/stroke 255 77 77)

             (q/ellipse (:x star) (:y star) (:size star) (:size star))))))

  (doseq [bonus (:bonus state)]

    (q/image (q/load-image (:image bonus))

             (:x bonus)

             (:y bonus)

             45 45))

  (q/image (:image (:rocket state))

           (:x (:rocket state))

           (:y (:rocket state)))

  (q/fill 0 0 255)

  (q/text-align :left)

  (q/stroke 0 0 255)

  (doseq [meteor (:meteors state)]

    (q/image (q/load-image "images/meteor.png")

             (:x meteor)

             (:y meteor)))

  (doseq [smoke (:smoke state)]

    (let [age (:age smoke)

          size (max 0.0 (- 10.0 (* 5.0 age)))

          [r g b] (:col smoke)

          [x y] (:pos smoke)]

      (q/fill 0 0 250 150)

      (q/stroke 0 0 250 150)

      (q/ellipse x y size size)))

  (q/fill 255 255 255)

  (q/text-size 20)

  (q/text (str "Score: " (:score state)) 10 30)

  (q/text (str "Highscore: " (:highscore state)) (- (q/width) 140) 30)

  (q/fill 200 0 0)

  (q/text-font (q/create-font "DejaVu Sans" 40 true))

  (q/text-align :center)

  (when (:gameOver state)

    (q/text (str "Game Over...nMove to try again") (/ (q/width) 2) 500)))



; update method

(defn update-state [state]

 (-> state

  (update-in [:meteors] (fn [meteors] (doall (map move-meteors meteors))))

  (update-in [:rocket] move-rocket)

  ; (update-in [:bonus] (fn [bonus] (doall (map move-bonus bonus))))

  move-stars

  create-new-star

  (update-in [:stars] remove-stars)

  emit-smoke

  (update-in [:smoke] (fn [smokes] (map age-smoke smokes)))

  (update-in [:smoke] remove-old-smokes)

  meteor-out

  create-meteor

  ; bonus-out

  create-bonus

  bonus-hit

  meteor-hit

  reset-game

  (update-in [:smoke] fly-backwards state)

  (update-in [:gameOver] reset-game-over state)))



;; defsketch

(q/defsketch rocket_game

  :host "host"

  :title "rocket game"

  :size [600 700]

  :setup setup

  :draw draw

  :key-pressed move

  :update update-state

  :middleware [m/fun-mode])

For code written by students new to the language, this is quite good. You haven't made any of the major mistakes that most newbies to the language make, like abuse of def, or use of unnecessary side effects.

For your main questions:

1. It's very possible to write imperative, non-functional code in Clojure. Since Clojure is heavily functional leaning however, non-functional code is usually quite ugly and verbose. As soon as you start using do (or a macro that emits a do), know that you've crossed a line. It's not necessarily a bad line, but one that means you're leaning imperative and relying on side effects instead of just passing data around explicitly.




2. In my personal library, I actually created my own versions of all the random functions that also accept a Java Random object. That way, if you pass the same random number generator into a function, it will return the same results. I don't like Clojure's standard random functions because they can't be seeded. I consider them to be a mistake, but that's just me. To answer your question however, it's always a balance. You can make them pure at the cost of an extra parameter, or they can be impure and harder to test, but easier to write. I have both ways, depending on the context.



3. 
   

   Use of an atom internally here is fine. You have (mostly) pure functions that represent a transformation of a state, and the state you're using is immutable. The fact that Quil is using an atom behind the scenes is irrelevant.

   
   
   

   Think of it this way, this useless function is pure, and the input and output are both immutable, despite the use of the atom internally:

   
   
   

   (defn my-func [n]
   
     (let [a (atom 0)]
   
       (doseq [m (range n)]
   
         (swap! a + m))
   
   
   
       @a))
   
   

   
   
   

   From the outside, the fact that this function uses an atom as an implementation detail changes nothing.

   
   



4. No, actually, this is quite functional. The main problem is the use of random numbers as mentioned in 2., but as you pointed out, that's hard to avoid. Ask yourself: "Is this function carrying out side effects, or just taking some data and returning some data?". If it's the latter, it's likely functional. There's other aspects of functional programming, but that's a big one.

Now onto a more thorough review:

I don't actually see anything glaringly wrong here! I'll comment on how this can be improved, but this isn't outright bad code. I'm going to basically just work top-down here.

First, probably one of the biggest problems here is the gross use of Magic Numbers. Take a look at star-color-index and inside?. How many seemingly arbitrary numbers do you have in those functions? If you came back to this code a year from now, would you remember what each number individually means? If you needed to make changes, would you being able to reliably? Bind the numbers in a let to give them a name, or if they're used in other functions as well, define them top level using a def, then refer to them as needed. Then, any changes you make to the number will automatically be reflected everywhere, and the names make it obvious what the numbers are even for.

You're using bare maps in a lot of places where a record might be a better option. Note how your star and game state "classes" always have the same fields. If you ever find yourself using a map that should only have very specific fields, use defrecord instead. It has the potential to improve performance, but more importantly, it makes it clear what data should and shouldn't be a part of the map. I'd change your create-star to:

; This clearly defines what a Star should have

(defrecord Star [x y size speed color])



(defn create-star [y]

  ; ->Star is an automatically generated constructor

  ; You could also use map->Star if you wanted to create it based on an existing map

  (->Star (rand-int (q/width))

          (rand-int y)

          (+ (rand-int 5) 1)

          (+ (rand-int 3) 1)

          (star-color-index (rand-int 20))))

Then you can do a similar thing with your main state. Anyone reading over the code can just read the defrecord at the top, and immediately know what data each structure holds.

I feel reset-state-variable could probably be written much neater using assoc. That way, you aren't needing to write things like :stars (:stars state). I'd also make use of destructuring so you don't need to write things like (:score state), and use max to decide what the new highscore is:

(defn reset-state-variable [state]

  (let [{:keys [score highscore]} state]

    (assoc state

           :rocket {:image (q/load-image "images/rocket.png")

                    :x 260

                    :y 340

                    :dir 0}

           :background (q/load-image "images/stars.jpg")

           :fires 

           :smoke 

           :score 0

           :highscore (max score highscore) ; A little more straight-forward

           :gameOver true

           :meteors 

           :bonus )))

The gain here using assoc isn't huge. Mainly just now you don't need to re-associate :stars. You can see real gains though if you look at meteor-out. Many of the fields you're just copying from the old state to the new one! Just use assoc so you don't need to handle stuff that isn't changing. I'm also going to sprinkle use of update in there since :score depends on its old value:

(defn meteor-out [state]

  (let [old-count (-> state :meteors (count))

        new-meteor (remove item-inside? (:meteors state))

        new-count (count new-meteor)]

    (-> state

        (assoc :background (q/load-image "images/stars.jpg")

               :gameOver false

               :meteors new-meteor)



        (update :score #(+ % (- old-count new-count))))))

Much less duplication. Oh, and don't use new as a symbol. That's actually a special operator in Clojure like it is in Java. I'm surprised that that doesn't raise an error actually.

I'll just quickly point out that :gameOver should really be :game-over. Dash separation is idiomatic Clojure, and camelCase isn't known for lending itself to readability.

meteor-hit is huge and very convoluted looking. It took me a second to figure out what was going on. I can't say I've ever written (if (loop... before. You're also using if to return true and false instead of just returning/negating the original condition. I'd break the function up, and use cond instead of nested ifs. I'll admit though, I tried to refactor the loop, and went into brain lock. It could definitely be neatened up, but it's significantly better just moving it out into it's own function:

(defn collision? [meteors rocket-x rocket-y]

  (loop [[m1 & rest] meteors]

    (if (or (and

              (<= (:x m1) rocket-x (+ (:x m1) 45))

              (<= (:y m1) rocket-y (+ (:y m1) 45)))

            (and

              (<= (:x m1) (+ rocket-x 45) (+ (:x m1) 45))

              (<= (:y m1) (+ rocket-y 45) (+ (:y m1) 45))))

      true

      (if (empty? rest)

        false

        (recur rest)))))



(defn meteor-hit [state]

  (let [rocket-x (-> state :rocket :x)

        rocket-y (-> state :rocket :y)

        meteors (:meteors state)]

    (cond

      (empty? meteors) state

      (collision? meteors rocket-x rocket-y) (reset-state-variable state)

      :else state)))

Just for emphasis on why you should really be using assoc, here's bonus-out when you use assoc:

(defn bonus-out [state]

  (if (item-inside? (:bonus state))

    state

    (assoc state

           :background (q/load-image "images/stars.jpg")

           :gameOver false

           :bonus )))

Again, much less duplication, and, if you ever added a field to the state, you no longer need to go back and fix every function that manipulates the state! That alone is massive.

There's no point in using update-in if you aren't actually accessing a nested member. age-smoke could/should be written just as:

(defn age-smoke [smoke]

  (update smoke :age #(+ % 0.3)))

You can also make use of update's var-arg overload. It seems complicated at first when you're reading the docs, but basically how it works is the value being updated is automatically passed as the first argument to the function. Any arguments after that are given to the function after the first argument (that sounds awkward). To clear it up, here's the same function without the wrapper function:

(defn age-smoke [smoke]

  (update smoke :age + 0.3))

% is automatically treated as the first argument to +, then 0.3 is given after it. This is nice if the update is already in a #(), since those can't be nested.

Nice use of remove. Gotta love code that reads like:

(defn remove-old-smokes [smokes]

  (remove old? smokes))

This could theoretically be "reduced" down to:

(def remove-old-smokes

  (partial remove old?))

This is a nice thing to think about, but I wouldn't stick with using partial in this particular example.

It looks like your "smoke" map should also be a record instead of a standard map.

For reset-game, I honestly prefer the condition to if to be on the same line as the if. I find it reads much nicer. I'd also bind :rocket in a let so you don't need to write :rocket twice:

(defn reset-game [state]

  (let [rocket (:rocket state)]

    (if (inside? (:x rocket) (:y rocket))

      (reset-state-variable state)

      state)))

You could also destructure out the :x and :y:

(defn reset-game [state]

  (let [{:keys [x y]} (:rocket state)]

    (if (inside? x y)

      (reset-state-variable state)

      state)))

This is a matter of style. It depends on where you want the bulk to be. In the body, or up in a let?

reset-game-over violates a personal pet-peeve of mine. Why use an if then just return true/false? You're also not using the gameOver parameter, and the name is wonky. That function isn't resetting anything; it's deciding if it should be reset. Just write:

(defn reset-game-over? [state]

  (-> state :rocket :dir (not= 0) (not)))

That double not looks a little off though. I'll admit I don't fully understand why dir is being used to decide if you should reset the game over state, but it might be possible to just write:

(defn reset-game-over? [state]

  (-> state :rocket :dir (zero?)))

In move-rocket, case doesn't require lists around what you're matching on. You can just use bare numbers there.

In your draw method, you're using a lot of nested ifs, and I feel that it's hurting readability. I'd use cond here instead to reduce nesting.

The functions that I didn't mention either had similar problems to one I'd already mentioned, or didn't have anything worth noting.