code-reviews

Enrico Teterra 2020-06-30T18:46:40.059200Z

👍 1
seancorfield 2020-06-30T18:51:15.060900Z

(s/invalid? (s/conform :todo/task-event event)) -- it's probably clearer to check (s/valid? :todo/task-event event) (and switch the if/else clauses). You normally only use s/conform if you actually want to conform the data (i.e., get back a data structure labeled with how it matches the spec).

👍 1
seancorfield 2020-06-30T18:52:47.061900Z

Look at multimethods as a more idiomatic way to write apply-event -- since it is essentially polymorphic on :event/name

👍 1
seancorfield 2020-06-30T18:55:01.062700Z

(when (s/valid? (s/keys :req [:event/name]) event) -- I'm curious why this isn't checking against :todo/task-event like the apply-event function?

seancorfield 2020-06-30T18:56:05.063700Z

Overall, nice and clean and easy to read. Good job!

🙂 1
1
phronmophobic 2020-06-30T18:56:28.064100Z

the formatting on apply-event is confusing to me. tasks is on the same line as the result of "task-completed" even though it's an else statement. I prefer a format like:

(defn apply-event [tasks event]
  (if (s/invalid? (s/conform :todo/task-event event))
    tasks
    (case (:event/name event)
      "task-added"
      (->> (select-keys event [:task/uri :task/title])
           (conj (remove #(= (:task/uri event) (:task/uri %)) tasks)))
      "task-completed"
      (remove #(= (:task/uri %) (:task/uri event)) tasks)

      ;; else
      tasks)))

dominicm 2020-07-01T08:09:21.085400Z

Just to expand on this, exceptions are the only error handling mechanism clojure has. Also nil punning for some cases. In clojure, exceptions are not reserved for shutdown/panic situations

seancorfield 2020-06-30T18:57:12.064800Z

Oh, good catch! I missed that completely.

phronmophobic 2020-06-30T18:57:31.065500Z

not sure whether it's good practice or not, but I often have comments for else statements in conds and cases

Enrico Teterra 2020-06-30T18:58:00.066400Z

ah good catch! i wanted to leave the event store open to also process other types of events that might be added in the future (there's no requirement on :task/uri or :task/title ) maybe i could add a smaller spec which i then compose into :todo/task-event and reuse there for the check

phronmophobic 2020-06-30T18:59:08.068Z

additionally, I don't think apply-event should return the tasks unmodified if an unrecognized event is sent. I would probably either remove the else statement, throw an exception, or return some sort of error

seancorfield 2020-06-30T19:00:58.070200Z

My (deleted) comment about adding a docstring still stands (I think everything should have a docstring really, including ns) -- but in task-event-handler I'd name the argument send-fn to avoid shadowing the built-in send (in clojure.core) which confused me when I read the body.

👍 1
phronmophobic 2020-06-30T19:01:37.070900Z

if I send an event to an API and I send it an invalid or unrecognized event, I would hope to receive an error rather than it silently do nothing.

seancorfield 2020-06-30T19:01:49.071100Z

That sounds good. Having an anonymous spec just for s/valid? seemed a bit odd.

seancorfield 2020-06-30T19:02:44.072Z

^ although adding error handling throughout the app is whole 'nother layer of complexity which is probably a learning exercise in its own right 🙂

phronmophobic 2020-06-30T19:04:30.073400Z

right. the simplest error handling implementation would be to simply remove the case's else statement and let the generic error handling take over

phronmophobic 2020-06-30T19:05:13.074900Z

it's not the best error handling, but at least it would indicate something went wrong to the API's user

Enrico Teterra 2020-06-30T19:08:18.077200Z

good point! you're right if the application reducer encounters an error it could lead to an incorrect state being created from the events - i'll have a think about that

Enrico Teterra 2020-06-30T19:08:34.077600Z

thanks for the pointers! really appreciate the help

Enrico Teterra 2020-06-30T19:10:27.077700Z

that is a lot more readable, yeah on the if its not all that clear what's happening sometimes, adding comments is a good idea. thanks for the tip

phronmophobic 2020-06-30T19:11:43.078300Z

apply-event combines validation and the event processing. my intuition is that those should be separate

phronmophobic 2020-06-30T19:13:10.078700Z

currently, apply-event will continue to grow in size if you keep adding more events. check out https://clojuredocs.org/clojure.core/defmulti for an alternate approach

👍 1
🙌 1
Enrico Teterra 2020-06-30T19:13:16.078900Z

right - in other languages I would sometimes start a function with some guards that exit early (like using :pre i guess? - but i didn't want to throw an error) that was my intention there

Enrico Teterra 2020-06-30T19:13:26.079100Z

i'll have a think where i could put it

phronmophobic 2020-06-30T19:17:27.079700Z

what's the reluctance to throwing an error?

Enrico Teterra 2020-06-30T19:19:35.080Z

i'm not sure if its an exceptional (unexpected) thing, it could happen that the event store contains some old events or stale events, or events with an old schema that shouldn't be reduced

Enrico Teterra 2020-06-30T19:19:56.080200Z

i prefer to reserve exceptions for things that should stop the application

phronmophobic 2020-06-30T19:21:19.080400Z

you could apply-events return some error-value rather than throwing an exception

👍 1
phronmophobic 2020-06-30T19:21:51.080600Z

the issue with ignoring invalid events is that ignoring events is unlikely to be the right course of action

phronmophobic 2020-06-30T19:22:16.080800Z

it means there's probably an issue that needs to be addressed and silencing the problem just makes it harder to track down

Enrico Teterra 2020-06-30T19:22:55.081Z

makes sense, at the very least a warning should be logged somewhere and maybe someone gets an alert

Enrico Teterra 2020-06-30T19:23:15.081200Z

i was thinking of leaving the event store open, so another producer could add a new type of event at any point

Enrico Teterra 2020-06-30T19:23:41.081400Z

but that would break my application then if i returned an error

phronmophobic 2020-06-30T19:23:55.081600Z

for a more rigorous approach, I recommend http://erlang.org/download/armstrong_thesis_2003.pdf very highly

phronmophobic 2020-06-30T19:24:17.081800Z

my point is that your application is already broken

phronmophobic 2020-06-30T19:24:26.082Z

ignoring the error doesn't fix it

Enrico Teterra 2020-06-30T19:25:03.082200Z

haha 🙂 thats a fair point, added it to the reading list thanks

phronmophobic 2020-06-30T19:26:57.082500Z

I was working on a mobile game where you run a virtual restaurant. if the game received a recipe it didn't recognize, rather than crashing , it just used the first recipe in the list of recipes. one day, we pushed an incorrect config and all our player's dishes turned into veggie burritos

😆 2
phronmophobic 2020-06-30T19:27:42.082700Z

🌯 🌯

Enrico Teterra 2020-06-30T19:30:59.083Z

i have to admit i'm not sure what the best practice is here in the event sourcing case, i'll need to look into it

seancorfield 2020-06-30T19:34:44.083500Z

You can always reify exceptions and treat them as events, by which I mean you can catch them, call Throwable->map on them, and transform them into events -- if you want.

🤯 1
😍 1
Enrico Teterra 2020-06-30T19:35:40.083700Z

ooh thats a nice idea

seancorfield 2020-06-30T19:35:48.084Z

I have quite a bit of code that does something similar to (try body (catch Throwable t {:exception (Throwable->map t)}))

seancorfield 2020-06-30T19:36:16.084200Z

Then you can pass it around like data and still tell exceptions from "real" data.