Adding augmented maps to a vector based on regex text grabbing

Question

This is my latest brain child for clojure, could it be done with less code, cleaner, or in a more re applicable way? I am at a pretty early stage in my clojure learning so an in depth explanation of how and why my code is faulty would help me get a handle on what I am doing wrong so I do not build bad habits.

(def next-apendage
  #(hash-map :name (clojure.string/replace (:name %1) #"[0-9]" (str (+ 1 %2 (read-string (re-find #"[0-9]" (:name %1))))))))

(defn add-apendages
  [times part]
  (loop [done 0
         final-apendages [part]]
    (if (= done times)
      final-apendages
      (recur
       (inc done)
       (conj final-apendages
             (next-apendage part done))))))

(add-apendages 2 {:name "tentacle-3"}) 
;;OUTPUT->[{:name "tentacle-3"} {:name "tentacle-4"} {:name "tentacle-5"}]

That really ugly looking first function takes two arguments a map and a number. The number is added (with a compensation for done starting at 0) to the :name string of the map. The second function condenses the new appendages along side the original into a list. The third section is a call to the second function.

Answer 1

First off, good job on your formatting! I often see people who are new to Lisp trying to write and format their code as if it were Java, so props to you for not doing that.

You've misspelled "appendage" as "apendage" in your code, so my first change would be to correct that misspelling.

Since you are using a function from clojure.string, your code would be more readable if you were to :require that namespace and alias it:

(ns appendages.core
  (:require (clojure [string :as s])))

Now you can replace, for instance, clojure.string/replace with just s/replace. You could also use a longer alias such as str if you find s to be too cryptic.

As you've noted, next-appendage is quite ugly. This is partially because you wrote it as a literal anonymous function instead of taking advantage of defn:

(defn next-appendage
  [part done]
  (hash-map :name (s/replace (:name part) #"[0-9]" (str (+ 1 done (read-string (re-find #"[0-9]" (:name part))))))))

A few comments on usage here:

• Instead of calling hash-map, you should just write a literal map with your key-value pairs.
• You should never use read-string unless you have an extremely good reason, because it can evaluate arbitrary code. Since you're simply using it to parse an integer, you should use something like Long/parseLong instead.
• All your next-appendage function really does is get the value in part at :name, transform that value in some way, and return a new map with the :name associated with that new value. This is exactly the use case for the update function.
• Since the code in next-appendage is so deeply nested, its readability would benefit greatly from use of the ->> macro.

With these changes, next-appendage would look something like this:

(defn next-appendage
  [part done]
  (update part :name
          #(->> (re-find #"[0-9]" %)
                Long/parseLong
                (+ 1 done)
                str
                (s/replace % #"[0-9]"))))

The add-appendages function can be greatly simplified. You're basically just looping through several values of done (which brings to mind the range function) and, for each of those values, calling next-appendage on part (like map), and finally assembling the result into a sequence with part at the beginning (cons):

(defn add-appendages
  [times part]
  (cons part (map (partial next-appendage part) (range times))))

You could make this even simpler by having replacing done with just the number that you're adding to the digit in the :name of part, which removes the need for the + 1 in next-appendage and the cons in add-appendages.

Answer 2

Some things that stand out from your code:

1. (def next-appendage #()) is really (defn next-appendage [part done])
2. Do not use anonymous functions except for really really short functions. You next-appendage function is long for Clojure standards.
3. Instead of hash-map you can use the map literal directly `{:name (clojure.string/replace ,,,}
4. Even though it seems cool when you are starting Clojure, one liners are very hard to read. You already know that is "really ugly" :)
5. It is unusual to use loop. Most of the time you can replace it with a simpler map or similar
6. Do not use read-string, unless you know what you are doing

My version would look like:

(defn add-apendages [times part]
  (let [ ;; find the prefix and initial number
        [_ prefix number-str] (first (re-seq #"(.*)-([0-9]+)$" (:name part)))
         ;; parse the number, using Java interop
        initial-number (Integer/parseInt number-str)]
    ;; Build the list of results using a simple map operation
    (map (fn [n] {:name (str prefix "-" n)})
         (range initial-number (+ 1 initial-number times)))))

Answer 3

I think next-appendage is too long/complex for an anonymous function, why not make it a regular function. Also you might use a let clause to make the logic easier to understand and avoid repeating (:name %1):

(defn next-apendage [inputMap toAdd]
  (let [oldName (:name inputMap)
        newNumber (str (+ 1 toAdd (read-string (re-find #"[0-9]" oldName))))
        newName (clojure.string/replace oldName #"[0-9]" newNumber)]      
    (hash-map :name newName)))