Fork me on GitHub
#code-reviews
<
2020-12-11
>
noisesmith16:12:49

a small thing: :false and :true should probably be false and true - then your macro can just be (if logging-enabled (prn ~message))`

noisesmith16:12:03

the nil is implicit, and message needs to be unquoted

noisesmith16:12:01

in the style of your previous macro, (prn ~message)` could also be (list prn message)` the two are equivalent

roelof17:12:44

@noisesmith thanks, I see now only a the message (prn message) and not (prn :hi) as the challenge said the outcome should be

roelof17:12:02

and a message that the else branch is missing

noisesmith17:12:42

wait, who says an else branch is missing?

noisesmith17:12:45

it's optional

noisesmith17:12:59

I disagree with clj-kondo, but it's clj-kondo (a style tool) telling you this

lukas.rychtecky08:12:02

Use when instead of if when you need to return nil from else branch.

noisesmith16:12:29

this is a controversial suggestion - many think that when is implicitly for side effects (I could go either way, but wouldn't go far as saying all one armed ifs should be replaced with when - if allows a single arm for a reason)

lukas.rychtecky17:12:30

I have never seen using when just for side effects. I just use when instead of (if test branch nil)

lukas.rychtecky17:12:43

Or maybe I just have got used to it because I use kibithttps://github.com/jonase/kibit

noisesmith17:12:44

I usually use (if test branch) or (and test then) for values, I definitely never use (if test branch nil)

lukas.rychtecky08:12:18

When I see (if test branch) I have feeling that the author forgot to add else 😄

noisesmith17:12:15

it's harmless to add a nil else branch, but it is also redundant

noisesmith17:12:41

(ins)user=> (if false :OK)
nil
(ins)user=> (if false :OK nil)
nil

roelof17:12:42

it runs so I do not have a problem with it

roelof17:12:34

the only thing that worries me that I see prn message as output instead of (pnr :hi) as stated in the challenge

noisesmith17:12:17

your original will show that:

(defmacro log [message]
  (if (= logging-enabled :true)
    `(prn message)
    nil))
my version won't:
(defmacro log [message]
  (if logging-enabled
   `(prn ~message)))
I think you skipped part of my feedback

noisesmith17:12:36

notice the ~ to unquote message

nate17:12:06

change the if to a when to mollify clj-kondo

noisesmith17:12:58

or and if you think when is only for side effects and don't mind a random false instead of nil :D

roelof17:12:16

@noisesmith then I see (prn ~ message) as output

noisesmith17:12:55

are you sure you aren't using ' instead of ` ?

noisesmith17:12:31

the ~ should not show up in the macro expand, it's a special instruction to the ` reader macro

roelof17:12:10

yes, im sure

roelof17:12:14

(def logging-enabled true)

(defmacro log [message]
  (if logging-enabled
    '(prn ~message)))


(macroexpand `(log :hi))

noisesmith17:12:24

that's not `, it's '

noisesmith17:12:25

it's the wrong one

roelof17:12:04

oke, this code works

roelof17:12:27

thanks for the feedback

noisesmith17:12:57

glad I could help