Fork me on GitHub
#code-reviews
<
2020-07-02
>
stopa18:07:42

Hey team, created a quick clojure service "journaltogether" • idea is: service sends an email to a few friends, asking how their day was • they all answer • the next day, we receive a summary Haven't used clojure in a while -- would love a clojurian's take on the code. Esp on uses of futures, abstractions, etc https://github.com/stopachka/jt/pull/1/files (~about 300 loc)

noisesmith18:07:59

any exceptions or errors inside the future calls will be caught by the future, and only get rethrown if you attempt to access the return value of the future

👍 3
noisesmith18:07:16

the common thing, if you don't need the return value, is to use try/catch with an error level log on failure

👍 3
noisesmith18:07:45

if the return value is in any way useful, you can ensure you see the exception by ensuring that deref occurs if it is realized?

stopa18:07:28

gotcha! so to make sure something like this: https://github.com/stopachka/jt/pull/1/files#diff-0bbeb00ad8db9e2d48dfa64de4f775ecR104 is a fine way to "reject" a promse • (i can just throw the error, if someone who is using it derefs, they will get it)

noisesmith18:07:56

it might be clearer to have a special "failure" value for the promise

noisesmith18:07:04

also here: https://github.com/stopachka/jt/pull/1/files#diff-0bbeb00ad8db9e2d48dfa64de4f775ecR101-R102 just a small style thing, into takes an optional transducer arg that can keywordize a key, you don't need to do it in two steps:

(into {} (map (fn [[k v]] [(keyword k) v])) src)

noisesmith18:07:58

of course you'd want that fn to actually be something like (defn keywordize-kv ...) but the above version works in a repl for demonstration

noisesmith18:07:31

in fact- now that I think a few seconds longer, try just using keywordize-keys without the into call?

stopa19:07:14

one thing i realized though: if i get a nested map from firebase, keywordize-keys won't work on it (I think this is because firebase returns something that isn't considered by keywordize-keys to be a map. will play around

noisesmith19:07:11

OK - then you'd need a recursive (into {} ...)

stopa19:07:36

(yes indeed! found smth nice, on it!)

noisesmith19:07:48

oh - I thought maybe it would be easy, came up with this:

(import (java.util Collection
                   Map))

(defn deep-into-map
  [transform node]
  (let [prepare (map (comp transform (partial deep-into-map transform)))]
    (cond (isa? (class node) Map)
          (into {} prepare node)
          (and (isa? (class node) Collection)
               (every? #(and (isa? (class %) Collection)
                             (= (count %) 2))
                       node))
          (into {}
                (comp (map vec) prepare)
                node)
          :else
          node)))

❤️ 3
stopa20:07:01

love it! ended up going with something v close to above -> https://github.com/stopachka/jt/blob/master/server/jt/core.clj#L31-L48

noisesmith20:07:39

the big gotcha with your version is that java allows maps with non-string keys, that's why I made the transformation object a function (that cold be specialized for your known input)

👍 3
stopa20:07:30

will look into adding smth similar, thank you!

noisesmith18:07:43

that will work in many cases

stopa18:07:57

:mindblown: will try! thank you!

noisesmith18:07:06

https://github.com/stopachka/jt/pull/1/files#diff-0bbeb00ad8db9e2d48dfa64de4f775ecR280 instead of using future here you can provide :join false to run-jetty

stopa19:07:28

great idea!

noisesmith18:07:46

your friends might not appreciate this being plain text in a public repo - you could put it in a resource file outside version control with configurable location https://github.com/stopachka/jt/pull/1/files#diff-0bbeb00ad8db9e2d48dfa64de4f775ecR148

stopa19:07:16

xD fair! will include in secrets