2
\$\begingroup\$

to learn Reagent, re-frame and spec I made a little personal project.

Description:

  • Given a list of Employees (name, date of when the mission end, and role of the employee), ouput a table with their information.
  • Add visual cue if the mission is already finished, soon to be finished (I choose 30days) or will not finish soon.
  • Add filters to the table (a bit like in Excel spreadsheet) for names and for roles

Here is the code

(ns my.reagent-examples
  (:require [reagent.core :as reagent]
            [cljs.spec.alpha :as s]
            [re-frame.db :as db]
            [re-frame.core :as rf]))

;; -- Specs --
;; -----------

(s/def ::name string?) ; Name is a string 
(s/def ::date-fin-mandat inst?) ; date is of type #inst (a timestamp) 
(s/def ::role keyword?) ;role is a keyword

; One people is a map with required keys name, date and role and no optional keys
(s/def ::people (s/keys :req [::name ::date-fin-mandat ::role] ;; ?Should I use :req or :un-req ?
                        :opt []))

; Peoples is a vector of 0 to many people
(s/def ::peoples (s/coll-of ::people :kind vector? :min-count 0))

; A filter is a 2 elements vector of keyword like [:role :dev]
(s/def ::filter (s/tuple keyword? (s/or :s string? :k keyword?)))

; Filters is a vector of 0 to many filter
(s/def ::filters (s/coll-of ::filter :kind vector? :min-count 0))

; Spec for the whole db
(s/def ::db (s/keys :req-un [::peoples ::filters]))

;; -- Data --
;; --------

(def peoples [
              {::name "julien" 
               ::date-fin-mandat (js/Date.) 
               ::role :dev}
              {::name "juscellino" 
               ::date-fin-mandat (js/Date. 2019 7 21) 
               ::role :dev}
              {::name "danny" 
               ::date-fin-mandat (js/Date. 2019 4 15) 
               ::role :dev}
              {::name "nathalie" 
               ::date-fin-mandat (js/Date. 2031 9 22) 
               ::role :rh}
              {::name "malik" 
               ::date-fin-mandat (js/Date. 2019 1 22) 
               ::role :analyste}
              {::name "daniel" 
               ::date-fin-mandat (js/Date. 2019 8 15) 
               ::role :dev}])



;; -- Helpers --
;; -------------
(defn filter-people
  [peoples filters]
  (into 
   [] 
   (set
    (flatten
     (mapv
      (fn [[k v]]
        (filter
         #(= (k %1) v)
         peoples))
      filters)))))



(def end-soon-style {:background-color "red"})
(def end-not-so-soon-style {:background-color "orange"})
(def end-very-not-soon-style {:background-color "green"})

(defn date+30
  [date]
  (js/Date. (.getFullYear date) (.getMonth date) (+ (.getDate date) 30)))

(defn date+90
  [date]
  (js/Date. (.getFullYear date) (.getMonth date) (+ (.getDate date) 90)))

(defn style-by-date
  [date end-soon end-not-so-soon end-very-not-soon]
  (let [today (js/Date.)]
    (cond 
      (>= today date) end-soon
      (< date (date+30 today)) end-not-so-soon
      :else end-very-not-soon)))

;; -- 1 - Dispatch event --
;; ------------------------

; Do i need this?
(defn dispatch-add-filter-event
  [new-filter]
  (rf/dispatch [:add-filter [:role :dev]]))



;; -- 2 - Event Handler --
;; -----------------------

;; -- Interceptor --
;; -----------------

; Interceptor for validating spec after every add into db
(defn check-and-throw
  "Throws an exception if `db` doesn't match the Spec `a-spec`."
  [a-spec db]
  (when-not (s/valid? a-spec db)
    (throw (ex-info (str "spec check failed: " (s/explain-str a-spec db)) {}))))

;; now we create an interceptor using `after`
(def check-spec-interceptor (rf/after (partial check-and-throw ::db)))

;; -- Define Event Handlers --
;; ---------------------------

; Initialize the app state
(rf/reg-event-db
 :initialize
 [check-spec-interceptor]
 (fn [_ _]
   {:peoples peoples
    :filters [[::role :dev] [::name "julien"]]}))

; Add a new filter in app state
(rf/reg-event-db
 :add-filter
 [check-spec-interceptor]
 (fn [db [_ new-filter]]
   (assoc db :filters (conj (:filters db) new-filter))))

; Add all filters for a particular key
(rf/reg-event-db
 :add-all-filters-for-key
 [check-spec-interceptor]
 (fn [db [_ key1 values]]
   (assoc db
          :filters
          (into 
           (:filters db)
           (map
            #(vec [key1 %1])
            values)))))


; Remove a filter from app state
(rf/reg-event-db
 :remove-filter
 [check-spec-interceptor]
 (fn [db [_ old-filter]]
   (assoc db 
          :filters 
          (filterv 
           #(not (= %1 old-filter))
           (:filters db)))))

; Remove all filters from app state for a particular key
(rf/reg-event-db
 :remove-all-filters-for-key
 [check-spec-interceptor]
 (fn [db [_ key1]]
   (assoc db
          :filters
          (filterv
           #(not (= (first %1) key1))
           (:filters db)))))


;; -- 3 - Effect Handler --
;; ------------------------

;; ?? Probably nothing here??

;; -- 4 - Subscription Handler --
;; ------------------------------

; Return the vector of filters
(rf/reg-sub
 :filters
 (fn [db _]
   (:filters db)))

; Return the peoples, unfiltered
(rf/reg-sub
 :peoples
 (fn [db _]
   (:peoples db)))

; Return a list of filtered peoples
(rf/reg-sub
 :peoples-filtered
 (fn [db _]
   (filter-people (:peoples db) (:filters db))))

; Given a key k, return a vector of values representing all the values present in peoples
(rf/reg-sub
 :values-for-key
 (fn [db [_ k]]
   (map #(k %1) (:peoples db))))

; Does filter contains the value k in it?
(rf/reg-sub
 :filter-contains?
 (fn [db [_ k]]
   (contains? (set (:filters db)) k)))
;; -- 5 - View Function --
;; -----------------------


(defn list-people
  []
  [:p (pr-str @(rf/subscribe [:peoples]))])


(defn list-people-filtered
  []
  [:p (pr-str @(rf/subscribe [:peoples-filtered]))])


(defn list-filter
  []
  [:p (pr-str @(rf/subscribe [:filters]))])


(defn button-add-role
  [role]
  [:button
   {:on-click (fn [e] (rf/dispatch [:add-filter [::role role]]))}
   (pr-str "Add filter" role)])


(defn button-remove-role
  [role]
  [:button
   {:on-click (fn [e] (rf/dispatch [:remove-filter [::role role]]))}
   (pr-str "Remove filter" role)])


(defn ressource
  [data]
  [:tr
   [:td (data ::name)]
   [:td {:style (style-by-date
                 (data ::date-fin-mandat)
                 end-soon-style
                 end-not-so-soon-style
                 end-very-not-soon-style)}(str (data ::date-fin-mandat))]
   [:td (data ::role)]
   ])


(defn checklist-filter
  [key1]
  (let [list-values (set @(rf/subscribe [:values-for-key key1]))]
    (into 
     [:form
      [:input
       {:type "checkbox"
        :id (str "all-" key1)
        :name (str "all-" key1)
        :on-change (fn [e] (if (.. e -target -checked)
                             (rf/dispatch [:add-all-filters-for-key key1 list-values])
                             (rf/dispatch [:remove-all-filters-for-key key1])))}]
      [:label {:for (str "all-" key1)} "All"]
      ]
     (for [value list-values]
       [:div
        [:input
         {:type "checkbox"
          :id value
          :name value
          :on-change (fn [e] (if (.. e -target -checked)
                               (rf/dispatch [:add-filter [key1 value]])
                               (rf/dispatch [:remove-filter [key1 value]])))
          :checked @(rf/subscribe [:filter-contains? [key1 value]])
          }]
        [:label {:for value} value]]
       ))))

(defn peoples-ui-filtered
  []
  (let [pf @(rf/subscribe [:peoples-filtered])]
    (into 
     [:table
      [:tr
       [:th "NAME"]
       [:th "DATE FIN MANDAT"]
       [:th "ROLE"]]
      [:tr
       [:td [checklist-filter ::name]]
       [:td ]
       [:td [checklist-filter ::role]]]
      ]
     (doall 
      (for [p pf]
        [ressource p])))))


(defn ui 
  []
  [:div
   [peoples-ui-filtered]
   ])

;; -- Kickstart the application --
;; -------------------------------

(defn ^:export run
  []
  (rf/dispatch-sync [:initialize])     ;; puts a value into application state
  (reagent/render [ui]        ;; mount the application's ui into ''
                  (js/document.getElementById "app")))

(run)

You can also look at this gist that has the same code in table.cljs, and also klipse.html which is a html page with Klipse setup to try the code in the browser.

As for the review, every feedbacks is appreciated but especially about re-frame and spec.

  • Did I understand correctly the separation of event handlers, effect handlers etc in re-frame?

  • Like I know that I did not use Dispatch Event much, where should I use those?

  • I did not use Effect Handler, but if I understand correctly, that would be the place for database query, localStorage operations, call to external API etc. No use in my project, am I correct?

  • Following the re-frame todomvc example, I added specs and a spec interceptor (copy-pasted from this example). Does my specs looks correct?

  • Is defining specs and an Interceptor something I should do in every project? Or is there case I should avoid it?

And finally, how is my overall clojure/script code?

I am aware that I should have many files in a real project (core, db, events, subs and views) but I coded in my browser with the help of Klipse so I put everything in the same file for now.

\$\endgroup\$
2
\$\begingroup\$

Unfortunately, I've never used spec, reframe or clojurescript before, but I can note general Clojure things that can be fixed up.

filter-people has far too much nesting. Having code like you do affects readability (you need to read it bottom-up instead of top-down like you would most code), and makes it harder to add to (as you'll need to need adjust indentation and be careful with bracket addition). It would be much cleaner if you used ->> (the thread last macro) instead:

(defn filter-people
  [peoples filters]
  (->> filters
       (mapv
         (fn [[k v]]
           (filter #(= (k %1) v) peoples)))
       (flatten)
       (set)
       (vec))) ; Instead of (into [])

Your date+ functions have unnecessary duplication. I'd add a parameter for days to generalize them:

(defn date+days [date days]
  (js/Date. (.getFullYear date) (.getMonth date) (+ (.getDate date) days)))

If you really wanted a date+90 and other such functions, define them in terms of the generalized function:

(defn date+90 [date]
  (date+days date 90))

Now if you need to make changes to the implementation, you aren't needing to change several functions.


If I were you, I'd indent a little more. You have a single space of indentation in many places. Code like

(fn [db [_ old-filter]]
  (assoc db 
         :filters 
         (filterv 
          #(not (= %1 old-filter))
          (:filters db)))))

Is harder to read than it needs to be. I would prefer putting small anonymous functions on the same line as the function they're being passed to, and use at least 2 spaces to indent. In this case, I might also use a let binding the break it up a bit so it's easier to process what's going on. Something closer to:

(fn [db [_ old-filter]]
  (let [filtered (filterv #(not (= %1 old-filter))
                          (:filters db))]
    (assoc db :filters filtered)))

Also note, (filter #(not ... is common and has a shortcut function called remove. You could make use of it here:

(remove #(= %1 old-filter) (:filters db))

Although this returns a lazy sequence and there is no removev version so if you definitely need a vector, you'll need to add a call to vec, or just use the version you had.


Here

(assoc db
       :filters
       (into 
        (:filters db)
        (map
         #(vec [key1 %1])
         values)))))

You're associating to db, but the new value is dependent on the old value. When that's the case, it's often preferred to use update instead:

(fn [db [_ key1 values]]
  (update db :filters
             (fn [fs]
               (into fs (map #(vec [key1 %1]) values)))))

Unfortunately, the gain isn't huge here because you're already using a short-hand function macro (#()), and they can't be nested. It often leads to cleaner code though.


I'll just point out that here

(contains? (set (:filters db)) k)))

contains? is unnecessary. Sets can be invoked to test for membership.

((set (:filters db)) k)

In some cases, (not necessarily here; see below though), that can lead to cleaner code.

Putting it in a set just to test for membership though seems rather inefficient. If it was already in a set, ya, a membership test would be fast. With how you have it now though, you're doing a full iteration of (:filters db) just so you can do an efficient lookup. A lookup of (:filters db) only requires a full iteration anyways though (in the worst case). I'd use some and check for equality:

(some #(= k %) (:filters db))

or, to use the above set-as-a-function tip:

(some #{k} (:filters db)) ; Use the set as a predicate

This will return the first true result, or nil (falsey) if a result isn't found. The early exit ensures it only does as much work as it needs to do.

\$\endgroup\$
  • \$\begingroup\$ Wow so much good tips here, thank you very much! For the last point, I had in mind to change filters to be a set instead of a vector, I don't think I would lose anything and that might be a bit better. But your way of doing with some is interesting! Especially with the tip for set membership. As for ->>, is there a particular nesting point where you switch from nesting to threading macros or are they like a must do in most situations regardless? And for every other good tips here, thank you very much! update in place of assoc looks good and remove looks interesting too:) \$\endgroup\$ – Julien Rousé May 9 at 19:33
  • 1
    \$\begingroup\$ @JulienRousé No problem. And for nesting limits, this is honestly something that I've gone back and forth on for the past couple years. I used to change it to threading macros once I had 4+ functions, but I've recently started starting at only 2 functions; especially if I think there's any chance that I'll need to add more functions to thread through later. I often write it with the macro from the start. I find threading macros to be much neater in really any circumstance when used properly. If you're transforming a list, you should probably be using ->> unless you're just using one function \$\endgroup\$ – Carcigenicate May 9 at 19:56
  • 1
    \$\begingroup\$ @JulienRousé A thing you may want to keep in mind when writing your own functions is to allow them to work with the macros well. The rule I stick to are if the functions transforms a list, put the list as the last argument (so it works well with map and filter chains), else, put the data to be transformed as the first argument (so it works well with -> chains that use functions like assoc) \$\endgroup\$ – Carcigenicate May 9 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.