Fork me on GitHub
#code-reviews
<
2015-11-06
>
martinklepsch11:11:55

I made a custom Clojure type! https://github.com/martinklepsch/custom-comparator-set First time I’ve done this. It’s pretty little code easy to review I guess. Comments on style/indentation/thinking/everything most welcome simple_smile

xeqi16:11:51

@martinklepsch: do you need to define .equals and .hashcode?

martinklepsch16:11:18

@xeqi: it probably would make sense simple_smile

martinklepsch16:11:29

I’m just doing that for the cljs side of things

xeqi16:11:39

I imagine a test case would be trying to use the sets as keys in a {}

xeqi16:11:38

also, tried to build locally to test and got "Could not find artifact adzerk:boot-test:jar:1.0.5-SNAPSHOT"

martinklepsch16:11:10

Yeah, that’s a local mod for cljc testing. PR outstanding in the boot-test repo.

martinklepsch16:11:19

Sorry about that

martinklepsch16:11:48

Thanks for the test case idea, was actually wondering how I can test this simple_smile

martinklepsch16:11:02

how’s your clojure/java post coming along btw simple_smile

xeqi16:11:32

I'm not 100% on that being a good testcase fwiw

xeqi16:11:56

haha, it will have to be several. That much stuff could easily be an ebook

ghadi16:11:38

martinklepsch: for the print-method do doseq on the vals with print-method recursively

martinklepsch16:11:17

@xeqi: is this what you had in mind kind of?

(deftest hashing
  (let [cset (ccset/custom-comparator-set :k {:k "a"})
        kmap (-> {} (assoc cset 1) (assoc cset 2))]
    (t/is (= 2 (get kmap cset)))
    (t/is (= 1 (count kmap)))))

martinklepsch16:11:42

(deftest hashing
  (let [cset1 (ccset/custom-comparator-set :k {:k "a"})
        cset2 (ccset/custom-comparator-set :k {:k "a"})
        kmap (-> {} (assoc cset1 1) (assoc cset2 2))]
    (t/is (= 2 (get kmap cset1)))
    (t/is (= 2 (get kmap cset2)))
    (t/is (= 1 (count kmap)))))
this is better

martinklepsch16:11:43

@ghadi: thanks, do you mean like this?

(defmethod print-method CustomComparatorSet [v ^java.io.Writer w]
  (.write w "#CustomComparatorSet{")
  (doseq [x (seq v)]
    (print-method x w)
    (.write w " "))
  (.write w "}"))

martinklepsch16:11:46

would I use interpose to add spaces between items?

ghadi16:11:07

this is good

ghadi16:11:15

you're doing imperative printing

ghadi16:11:27

keep it clear

martinklepsch16:11:51

@ghadi: if I want to elide the trailing space before the closing } I’d use loop and check for rest?

xeqi16:11:57

@martinklepsch yeah, something like that to see if you needed to override those methods. Did the test fail without custom .hashcode/.equals?

martinklepsch16:11:12

@xeqi: yes they’re failing

martinklepsch16:11:46

@xeqi: is there some place where I can lookup protocols/interfaces for clojure types?

martinklepsch16:11:36

like how do I figure out what Interface I have to implement for .hashcode and .equals?

xeqi16:11:24

Those two are on Object

martinklepsch17:11:22

@xeqi: where you would start with testing?

xeqi17:11:24

@martinklepsch I meant for finding protocols/interfaces for Clojure types. For finding out what classes/interfaces those methods belog to higher up, I don't know a good place

martinklepsch17:11:14

in the process of adding collection-check to the tests

xeqi17:11:03

Though if I was testing your set, i'd highly consider throwing collection-check at (comparable-set identity) compared to #{}

martinklepsch17:11:37

that’s exactly what I’m doing simple_smile

martinklepsch17:11:05

@xeqi: do you think comparable-set is a better name (just asking because you used it)

xeqi17:11:09

Though that might not be quite right, cause is (= (c-s identity) (c-s #(%1))? How does the comparable effect equality? Etc etc

xeqi17:11:02

I hadn't though about it, just typed that cause I'm on mobile atm and didn't want to go back to the code to find real name

xeqi17:11:49

I don't have a good term for this concept

xeqi17:11:40

@martinklepsch back to actual review, could you delegate .contains to (contains? data (comparator v))? I think that would avoid building a set and walking every element each time.

martinklepsch17:11:48

@xeqi: good suggestion thanks!