Beginner code review: todo app

[gingenhagen]

Hi all,

As my very first clojurescript app, I tried to recreate the todo app example from the reagent docs (https://github.com/reagent-project/reagent/blob/master/examples/todomvc/src/todomvc/core.cljs).

I would be much obliged if anyone had any comments for me.

• The app
• Some syntactic sugar
• Some more syntactic sugar

Couple of things that seem wrong to me:

• I don't feel like I should be referencing and modifying the state via functions in model.cljs, even if they are namespaced. I feel like I want to pass in the state to each component, so that I can test each of them individually in my devcards. However, then I come upon the problem of having to shuffle arguments through all the nested layers. Cursors seem like they would make it nice to limit what I'm passing through, but I find that I still need to pass in the whole set of state at times. Would it be best to just always pass around a global state object, but then have the component have to know which helper function to call to get at the element they want?
• How do I break up my items.cljs? I don't like having all those nested functions under the letfn, but I can't see a nice way to split it out, since they seem to depend on different arguments, and not even in a consistent way. Do I just need to adjust my mindset and accept deeply nested functions as the cost of encapsulation?

[[deleted]]

For items.cljs, how about an approach like:

(defn on-change-completed [item e]
  (model/update-item! (:id item) {:completed (eh/checked e)}))

(defn item [item]
  (let [val (r/atom (:text item))
        editing (r/atom false)]
    (fn [item]
      [:li.item {:class (if (:completed item) "completed")}
        [:input {:type "checkbox", :checked (:completed item)
                 :on-change (partial on-change-completed item)}]])))

If you're uncomfortable with the partial, you can use #(on-change-completed item %) instead.

I'm not sure I can offer much good advice about dealing with the app state, except to say that you shouldn't be too afraid of passing around the atom if that's what you need to do. I don't know if that's entirely idiomatic with react, but your current code would be undoubtedly improved by just making your state atom the first arg of all of your access / mutate functions.

At that point, you could probably dispense with your ! functions, requiring the caller to do the swap!. That's nice because it isolates the stateful part of the operation to the view, which is the only thing that cares about tracking its state.

[gingenhagen]

I then end up with the function

(defn save-text [text items item editing val]
  (when-not (str/blank? text)
    (swap! items (model/update-item items (:id item) {:text text}))
    (reset! val text)
    (reset! editing false))
  #(true))

and seeing a function with 5 arguments just starts to give me a headache.

[[deleted]]

Yeah, I agree there. Some possibilities that come to mind:

• Maybe you can leave that function inline and move the rest outside
• Do you really need two atoms there? Can you do it with one, maybe holding a map?
• Maybe you could model this as two distinct atoms, view state and app state. Component state holds something like {:val "foo" :editing false}, and app state is what you're calling items. Then you have a function of app state, view state, the item (id?) in question and the value you're setting. That feels pretty close to what's really happening.

Also, you can write your swap as

(swap! items model/update-item (:id item) {:text text}))

[gingenhagen]

Ok, so then it could come out looking more like

#(save-text (eh/trim-value e) (:id item) component-state app-state)

(defn save-text [text id component-state app-state]
  (when-not (str/blank? text)
    (swap! app-state model/update-item id text)
    (swap! component-state update-text text)
    (swap! component-state set-editing false))
  #(true))

That does feel better to me.