This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
- # admin-announcements (35)
- # beginners (18)
- # boot (257)
- # cider (14)
- # clojure (226)
- # clojure-china (1)
- # clojure-germany (2)
- # clojure-japan (91)
- # clojure-korea (3)
- # clojure-poland (11)
- # clojure-spain (37)
- # clojure-ukraine (1)
- # clojurescript (165)
- # code-reviews (33)
- # datomic (8)
- # editors (42)
- # euroclojure (2)
- # jobs (30)
- # ldnclj (72)
- # reading-clojure (5)
- # slack-help (5)
@kamillelonek: I’ve had great results with Gerrit for code reviews, but it’s a little bit more heavyweight than a one-off code review. gerrithub is free for OSS
seriously, I need a dead simple solution, something like GitHub gists, but with better commenting options, I'll stick to gists for now, but in case I find something I'll post it here
may I ask for a code review: https://gist.github.com/KamilLelonek/304699d8803e699bfd45
@kamillelonek: I would be careful of your naming, and try to make it a bit clearer -> declaring github-user-public-keys-url as a def but also in the parameter of a function for instance. Think about namespaces too, eveything in here is specific to github so I wouldn’t have github in all the defn’s.
could become something like
(defn github-user-public-keys-json [github-user-public-keys-url] (json/read-str (github-user-public-keys-response github-user-public-keys-url) :key-fn keyword))
(defn github-user-public-keys-json [public-keys-url] (let [response (response-body public-keys-url) ] (json/read-str response :key-fn keyword)))
github-user-public-keys-response makes more sense becoming response-body, and it is then re-usable and it doesn’t matter what you give to it (within reason)
I am not sure If json should be in the function also, as it is implied that json will be returned. Which it will not.
@gjnoonan: what do you think now: https://gist.github.com/KamilLelonek/304699d8803e699bfd45?
@kamillelonek: Sorry for the late reply, better, but still watch out for your naming.
For instance, is json needed in the name? I don’t feel it adds any value. It would be better mentioning that in the docstring.. member is being mentioned a lot in the functions, which implies to me it needs to be separated.. A few more little things too, but again relating to naming.
(defn- all-members-json  (:body (all-members-response)))
I feel that a verb/action needs to be the function of the map. What are you intending to do with
(defn- all-members-public-keys-urls  (map github/member-public-keys-url (all-members-logins)))
github/member-public-keys-urldoesn’t tell me.
in the first case when I'm using
json is to distinguish function name from the other one without
it's not needed at all, but I don't have a better name,
all-members already exists
naming is kinda hard for me, when I come from OO where it was easier to have objects and name their methods
Which is why I think something like a response-body function, which grabs the body of any response you pass into it may be a better option.
member-public-keys-with-id wheres the id from? does it take an id or is it returned with an id?