Clojurians Log v2

I have a function, where I need to do a few things: 1. fetch an "invitation" 2. from the invitation, fetch a bunch of other maps ("sender-user", "receiver-user", "group") 3. then i need to update the "group" and send an email To do this, I wrote code that looks like this:

(defn accept-invitation [id]
  (future
    (when-let [{:keys [sender-email group-id receiver-email] :as invitation} 
               @(db/get-invitation-by-id id)]
      (if-not (and sender-email group-id receiver-email)
        (log/warnf "skipping, invalid invitation %s" invitation)
        (let [sender-user (db/get-user-by-email! sender-email)
              receiver-user (db/get-user-by-email! receiver-email)
              group @(db/get-group-by-id group-id)]
          (if-not (and sender-user
                       receiver-user
                       group
                       (-> group :users (get (:uid sender-user))))
            (log/warnf "skipping invitation %s sender-user %s receiver-user %s group %s"
                       invitation sender-user receiver-user)
            (do
              (fut-bg @(db/delete-invitation id))
              @(db/add-member-to-group group-id receiver-user))))))))

a couple of thoughts: • it's a little weird that the whole body of the function is wrapped in a future. it might be more appropriate to allow the code that calls this function decide if it wants to be asynchronous or not • the fact that there multiple futures seems like it would be hard to debug this code if something goes wrong. • stateful code with lots of branching is generally difficult. it can sometimes be verbose. it looks out of place in compared to most clojure code which is succinct. I wouldn't feel too bad if some part of your code that truly must deal with side effects looks like this. (for example, https://github.com/clojure/clojure/blob/clojure-1.10.1/src/clj/clojure/genclass.clj#L124) • you've summarized your code with the 3 steps above, if it's possible to break your code down into those steps, that could help. even just adding the 3 steps as a comment is helpful for future readers • you're making db calls that sometimes are deref'ed and some which aren't. I would consider trying to make your db calls more consistent. generally, I think it's better to have functions that aren't asynchronous and allow consumers of db calls decide if they want to wrap it in a future or some other thing • there are a bunch of branches that end up short circuiting (eg. if @(db/get-invitation-by-id id) is nil, then the top level when-let doesn't do anything. I would consider treating those as errors and asserting. this will reduce the nesting/branching, and probably highlight issues more quickly. calling accept-invitation with a non existent id is probably an error and should be caught as early as possible eg:

(defn accept-invitation [id]
  (let [{:keys [sender-email group-id receiver-email] :as invitation}
        @(db/get-invitation-by-id id)

        _ (assert invitation (str "Invalid invitation id " id))
        _ (assert (and sender-email group-id receiver-email)
                  "skipping, invalid invitation %s" invitation)
        
        sender-user (db/get-user-by-email! sender-email)
        receiver-user (db/get-user-by-email! receiver-email)
        group @(db/get-group-by-id group-id)

        _ (assert (and sender-user
                       receiver-user
                       group
                       (-> group :users (get (:uid sender-user))))
                  "skipping invitation %s sender-user %s receiver-user %s group %s"
                  invitation sender-user receiver-user)]

    (fut-bg @(db/delete-invitation id))
    @(db/add-member-to-group group-id receiver-user)))

> it's a little weird that the whole body of the function is wrapped in a future. it might be more appropriate to allow the code that calls this function decide if it wants to be asynchronous or not great point! i think i've been a bit stuck on the single-thread view of the world -- hence providing "async functions" etc -- but indeed why make the decision at fn level. Will update, thanks! > the fact that there multiple futures seems like it would be hard to debug this code if something goes wrong. curious, why would it be hard to debug? (since all the futures are blocked) > you're making db calls that sometimes are deref'ed and some which aren't. I would consider trying to make your db calls more consistent. generally, I think it's better to have functions that aren't asynchronous and allow consumers of db calls decide if they want to wrap it in a future or some other thing indeed this is a side-effect of the fact that sometimes firebase gives me blocking calls, sometimes it returns futures. will just block futures, hence api can be consistent. Thanks! > there are a bunch of branches that end up short circuiting (eg. if `@(db/get-invitation-by-id id)` is nil, then the top level `when-let` doesn't do anything. I would consider treating those as errors and asserting. this will reduce the nesting/branching, and probably highlight issues more quickly. calling `accept-invitation` with a non existent id is probably an error and should be caught as early as possible love the assert idea -- your code def cleans this up! will update : }

> stateful code with lots of branching is generally difficult. it can sometimes be verbose. it looks out of place in compared to most clojure code which is succinct. as these are db calls, am not quite sure if there is a better way. would you use some different kinds of abstractions, or do you think this fits under the "stateful code" category?

> as these are db calls, am not quite sure if there is a better way. would you use some different kinds of abstractions, or do you think this fits under the "stateful code" category? yep. db calls are side effects. there's certainly ways to build abstractions, but my point was that this looks pretty reasonable and I wouldn't worry too much that it doesn't look as pretty as some of the clojure code which is purely functional

> curious, why would it be hard to debug? (since all the futures are blocked) :thumbsup: I wasn't sure if some calls were futures that weren't derefed.

awesome, thank you so much for the deep review Adrian!