Fork me on GitHub
#code-reviews
<
2019-02-28
>
alice00:02:55

Hey guys, I just made this thing https://github.com/fp-alice/cljmcs Aside from not having any tests, am I doing anything wrong?

noisesmith00:02:42

this could be expressed as (set/rename-keys x {:download :file :href :location}) https://github.com/fp-alice/cljmcs/blob/master/src/cljmcs/http/scraper.clj#L11

alice00:02:11

never seen that function before, good tip

noisesmith00:02:38

subjectively, sometimes (first (filter f coll)) is better than constructing a predicate that returns its input so you can use some https://github.com/fp-alice/cljmcs/blob/master/src/cljmcs/minecraft/servers.clj#L16

noisesmith00:02:47

the predicate there could be (comp #{minecraft-version} :version) - returns truthy if the value under the key matches any value in the set

noisesmith00:02:57

(that's also subjective)

alice00:02:42

How does that work?

alice00:02:50

i understand compose

noisesmith00:02:54

a keys is a function from a map to the value under the key

noisesmith00:02:07

a set is a function from a potential member to itself only if contained in the set

noisesmith00:02:29

(#{foo} foo) -> foo

alice00:02:49

Oh so it's like getting the version and then checking if it's equal to the given one with the set

noisesmith00:02:53

(#{foo baz} bar) -> nil

alice00:02:58

I always read comp backwards

jaihindhreddy02:02:16

Whenever I look at (comp f g h), I tend to see #(f (g (h ...))). Of course if you transform it to #(-> ... f g h) it will seem backwards.

Daniel Hines14:02:40

I had this same problem. But apparently every functional language that has compose does it from right to left.

noisesmith00:02:22

yeah, if the devs in your team (you) don't find that straightforward, it's probably a bad style choice

noisesmith00:02:29

but it's an idiom I personally like

alice00:02:51

I love function composition I just always have to read the sexps twice

noisesmith00:02:49

you use http://clojure.java.io here already, so you could use io/file, but you could also import java.io.File so that you can just use File https://github.com/fp-alice/cljmcs/blob/master/src/cljmcs/os/files.clj#L15

alice00:02:13

oops, io/file makes sense

noisesmith00:02:27

the portable version of this is (System/getProperty "user.dir") https://github.com/fp-alice/cljmcs/blob/master/src/cljmcs/os/shell.clj#L8

alice00:02:56

If I want to make it work on windows too I'll keep that in mind thanks for the tip

noisesmith00:02:44

an extra "print-generic-help" type thing as the odd numbered element at the end of the case will make more sense to the user than the runtime error clojure gives if no case is matched https://github.com/fp-alice/cljmcs/blob/master/src/cljmcs/core.clj#L14

noisesmith00:02:04

well, the getProperty is also a single method call, instead of a chain of functions as well

alice00:02:17

good point

alice00:02:36

thanks for the criticism it's really valuable to me 🙂

noisesmith00:02:54

it's all my opinions so far, no bugs

noisesmith00:02:01

in fact this

(if action
      (case action
        "download" (download-command/download! arguments)
        "list"     (list-command/list! (first arguments))
        "help"     (help-command/help!))
      (help-command/help!))))
could be (case action "download" (download-command/download! ...) "list" (list-command/list! ...) "help" (help-command/help!) (help-command/help!)) - both the lack of an action and an unrecognized action often map to the same help output in shell utils

alice00:02:35

Didn't think of that

noisesmith00:02:50

or for style you could differentiate the help output from unmatched by adding a prefixing "no matching command"