Fork me on GitHub
#code-reviews
<
2020-12-17
>
noisesmith19:12:32

> Use a promise to hand the result back out of the thread. Use this > ; technique to write your own version of the future macro. you made a function for this, not a macro, if that matters

noisesmith19:12:38

and as mentioned elsewhere, your atomSum doesn't use future correctly - it doesn't start one thread until the other completes

noisesmith19:12:13

also, since this is #code-reviews, I would recommend not using atom as the name for an atom

roelof19:12:07

oke, so I can change def future to defmacro future ?

noisesmith19:12:20

also, we don't use camelCase for clojure names, we use kebab-case

noisesmith19:12:41

well, defmacro works differently than defn, I don't know if you actually need to use a macro for this assignment

roelof19:12:44

and how I can I make atomSum work the right way ? I found this chapter very confusing

noisesmith19:12:07

deref blocks the code until the future exits

noisesmith19:12:53

if you have (let [a (deref (future ...)) b (deref (future ...))] ...) b doesn't start until a exits

roelof19:12:48

I had that but if I remember you told me to do this way because the a and b where not used error message

noisesmith19:12:48

if you have (let [a (future ...) b (future ...)] ...) they both run at the same time

noisesmith19:12:15

that's not an error message, that's your linter

noisesmith19:12:28

you still need to deref a and b, but after b is created

noisesmith19:12:44

(let [a (future ...) b (future ...)] (deref a) (deref b) ...)

noisesmith19:12:09

the deref here is just telling the code to wait for that block to exit (so the atom has the right total in it)

noisesmith19:12:44

also, if you aren't using deref as a first class function, or using its optional timeout option, just use @ which expands to deref

roelof19:12:44

So this is good clojure code :

(defn atomSum []
  (let [atom  (atom 0)
        part1 (deref (future (swap! atom +  (apply + (range 0 (/ 1e7 2))))))
        part2 (deref (future (swap! atom +  (apply + (range 0 (/ 1e7 2))))))]
    (deref atom)))

noisesmith19:12:13

the deref is blocking part2 so it doesn't start until part1 exits

roelof19:12:51

so I can ignore th linter message that part1 and part2 are not used

noisesmith19:12:14

you need to deref after part2 is created, or the atom will have the wrong sum

noisesmith19:12:27

(defn atom-sum []
  (let [a (atom 0)
        part1 (future (swap! atom + (apply + (range 0 (/ 1e7 2)))))
        part2 (future (swap! atom + (apply + (range 0 (/ 1e7 2)))))]
    @part1
    @part2
    @a))

roelof19:12:07

sorry, I miss you now

noisesmith19:12:56

deref means "block this thread until that other thread provides a value"

roelof19:12:57

this is not good :

(defn atomSum []
  (let [atom  (atom 0)
        part1 (future (swap! atom +  (apply + (range 0 (/ 1e7 2)))))
        part2 (future (swap! atom +  (apply + (range 0 (/ 1e7 2)))))]
    (deref part1)
    (deref part2)
    (deref atom)))

noisesmith19:12:17

what's wrong with it?

noisesmith19:12:28

(other than the naming stuff mentioned elsewhere)

roelof19:12:48

oke, then I miss that part

noisesmith19:12:21

also (range 0 x) is the same as (range x)

roelof19:12:28

because you said this : and as mentioned elsewhere, your atomSum doesn't use future correctly - it doesn't start one thread until the other completes

noisesmith19:12:57

right, you pasted a version that used deref inside the let block

roelof19:12:35

I thought that was on my code on gitlab

noisesmith19:12:04

oh, that looked like a paste, I didn't notice it was a preview from some plugin

roelof19:12:05

I see , there I have the version that uses deref

roelof19:12:48

a moment, I will update my gitlab repo

roelof19:12:52

oke, updated

roelof19:12:01

is it now good ?

noisesmith19:12:38

oh I see you had to fix the ranges too - yeah that looks better

roelof19:12:46

🙂

roelof19:12:07

rought ride to learn clojure but slowly i learn it

noisesmith19:12:49

also note that if x is a double (like your ocde) range makes longs if you ask for (range x), and makes doubles if you ask for (range x y)

noisesmith19:12:03

(ins)user=> (range 1e2)
(0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99)
(ins)user=> (range 1e2 2e2)
(100.0 101.0 102.0 103.0 104.0 105.0 106.0 107.0 108.0 109.0 110.0 111.0 112.0 113.0 114.0 115.0 116.0 117.0 118.0 119.0 120.0 121.0 122.0 123.0 124.0 125.0 126.0 127.0 128.0 129.0 130.0 131.0 132.0 133.0 134.0 135.0 136.0 137.0 138.0 139.0 140.0 141.0 142.0 143.0 144.0 145.0 146.0 147.0 148.0 149.0 150.0 151.0 152.0 153.0 154.0 155.0 156.0 157.0 158.0 159.0 160.0 161.0 162.0 163.0 164.0 165.0 166.0 167.0 168.0 169.0 170.0 171.0 172.0 173.0 174.0 175.0 176.0 177.0 178.0 179.0 180.0 181.0 182.0 183.0 184.0 185.0 186.0 187.0 188.0 189.0 190.0 191.0 192.0 193.0 194.0 195.0 196.0 197.0 198.0 199.0)

noisesmith19:12:16

and I think your code is the kind of code where long vs. double will change the result

noisesmith19:12:42

doubles are also slower to do math on than longs are

roelof19:12:15

oke, the 0 and the 1e7 were given by the challenges

noisesmith19:12:06

it looks like in this case it will give the same answer, but if you use the long function eg. (long 1e7) you can ensure it uses integral math

noisesmith19:12:51

it cuts the execution time in ~half

(cmd)user=> (time (long (apply + (range 0.0 1e7))))
"Elapsed time: 247.583409 msecs"
49999995000000
(ins)user=> (time (long (apply + (range 0 (long 1e7)))))
"Elapsed time: 120.047445 msecs"
49999995000000

roelof19:12:38

oke, so for math things I can better use longs ?

noisesmith19:12:04

it varies, sometimes you need floating point

noisesmith19:12:12

but in general longs are better (more accurate, faster)

roelof19:12:26

oke, thanks for the feedback

roelof19:12:37

im very slowly learning clojure

roelof20:12:15

tomorrow I hope I can solve the last one of this page : https://aphyr.com/posts/306-clojure-from-the-ground-up-state

roelof20:12:29

then I finishced this one after 3 - 4 days

roelof20:12:11

now time to sleep