clojure-more readable way to write this function?

Razz 0
Viewed 86

I have written this function to convert a vector of maps into string. There is a second map called field-name-to-columns which contains a mapping between the field-name and the actual name of columns in my database. My goal is to get a string like in the example where if the key is not present in the field-name-to-columns be ignored. Plus I want to have “client.name DESC” as a default if the :sorting key is empty or missing or none of the field-names matches any key in field-name-to-columns.

(def field-name-to-columns {"name" "client.name"
                            "birthday" "client.birthday" 
                            "last-name" "client.last-name" 
                            "city" "client.city"})

(def request {:sorting [{:field-name "city" :desc true} 
                        {:field-name "country" :desc true} 
                        {:field-name "birthday" :desc false}]})

(defn request-to-string
  "this function creates the sorting part of query"
  [sorting]
  (if (empty? sorting)
    (str "client.name" "DESC")
    (->> (filter some? (for [{:keys [field-name desc]} sorting]
                         (when (some? (field-name-to-columns field-name)) (str (field-name-to-columns field-name) (when desc " DESC")))))
         (st/join ", "))))

(request-to-string (request :sorting))

=>"client.city DESC, client.birthday"

Any comments on how to write this function more readable would be highly appriciated

code-readability clojure
3 Answers

What you've written is very reasonable in my opinion. I'd just add some whitespace for a visual break and tidy up your null handling a bit: it's silly to put nulls into the result sequence and then filter them out, rather than producing only non-nil values.

(defn request-to-string [sorting]
  (str/join ", "
            (or (seq (for [{:keys [field-name desc]} sorting
                            :let [column (field-name-to-columns field-name)]
                            :when column]
                       (str column (when desc " DESC"))))
                ["client.name DESC"])))

I've also moved the str/join up front; this is a stylistic choice most people disagree with me about, but you asked for opinions. I just think it's nice to emphasize that part by putting it up front, since it's an important part of the process, rather than hiding it at the back and making a reader remember the ->> as they read through the body of the function.

I also prefer using or rather than if to choose defaults, but it's not especially beautiful here. I also considered (or (non-empty (join ...)) "client.name DESC"). You might prefer either of these options, or your own choice, but I thought you'd like to see alternate approaches.

amalloy0

Here is one idea, based on my favorite template project.

(ns tst.demo.core
  (:use demo.core tupelo.core tupelo.test)
  (:require
    [tupelo.string :as str]))

(def field-name->columns {"name"      "client.name"
                          "birthday"  "client.birthday"
                          "last-name" "client.last-name"
                          "city"      "client.city"})

(defn field->string
  [{:keys [field-name desc]}]
  ; give names to intermediate and/or temp values
  (let [col-name (field-name->columns field-name)]
    (when (some? col-name)
      (str col-name
        (when desc " DESC")))))

(defn request->string
  "this function creates the sorting part of query"
  [sorting]
  ; accept only valid input
  (when-not sorting ; WAS:  (str "client.name" "DESC")
    (throw (ex-info "sorting array required, value=" {:sorting sorting})))

  ; give names to intermediate values
  (let [fiels-strs (filter some?
                     (for [entry sorting]
                       (field->string entry)))
        result     (str/join ", " fiels-strs)]
    result))

and unit tests


(verify
  (is= (field->string {:field-name "city", :desc true}) "client.city DESC")
  (is= (field->string {:field-name "country", :desc true}) nil)
  (is= (field->string {:field-name "birthday", :desc false}) "client.birthday")

  (let [sorting [{:field-name "city" :desc true}
                 {:field-name "country" :desc true}
                 {:field-name "birthday" :desc false}]]
    (is= (spyx-pretty (request->string sorting))
      "client.city DESC, client.birthday")))
Alan Thompson0

I prefer the (->> (map ...) (filter ...)) pattern over the for macro:

(defn request-to-string [sorting]
    (or (->> sorting
             (map (fn [{:keys [field-name desc]}]
                    [(field-name-to-columns field-name)
                     (when desc " DESC")]))
             (filter first)
             (map #(apply str %))
             (clojure.string/join ", ")
             not-empty)
        "client.name DESC"))
Markus Agwin0
Related