clojure: idiomatic way of removing duplication in an "if"?

Question

I'm very new to clojure, and I haven't done a ton of lisp previously. I have a function that contains the following:

(defn chord 
    ([scale degree num_voices]
    (if 
        (keyword? degree)  
            (take num_voices (take-nth 2 (cycle (invert scale (.indexOf scale degree)))))
            (take num_voices (take-nth 2 (cycle (invert scale degree))))))

Obviously, this code is poor because having two almost-identical function calls here is suboptimal, where the only difference is (.indexOf scale degree) vs degree.

What's the Clojure/Lisp Way of removing this code duplication? I feel like it should involve a let, but I'm not positive. Any other general pointers related to this code block are also appreciated.

Edit: I have re-factored the code according to andrew cooke's suggestion, the function now reads:

(defn chord
    ([scale degree num_voices]
        (let [degree (if (keyword? degree) (.indexOf scale degree) degree)]
            (take num_voices (take-nth 2 (cycle (invert scale degree))))
        )
    )

Thanks to everyone who answered so quickly.

At least for common-lisp (and I assume clojure as well), those last two parentheses are usually placed at the end of the (take...) line; with a good editor that properly indents lisp code, indentation will take the place of what you are doing with those last two parentheses. — Clayton Stanley, Apr 09 '12 at 16:06

Marcin · Answer 1 · 2012-04-10T06:08:02.690

6

if returns an expression, so invert the structure of your function:

(defn chord 
    ([scale degree num_voices]
    (take num_voices (take-nth 2 (cycle (invert scale (if (keyword? degree)
                                                              (.indexOf scale degree)
                                                           (invert scale degree))))))))

It would probably be even better if you used a let to capture the result of the if.

edited Apr 10 '12 at 06:08

answered Apr 08 '12 at 17:00

Marcin

48,559
18
128
201

thanks, upvoted, your answer is of course correct but the "let" does make it a bit more readable I think. – Paul Sanwald Apr 08 '12 at 17:10
@PaulSanwald Yes, that's why my answer says that. – Marcin Apr 08 '12 at 18:58

andrew cooke · Accepted Answer · 2012-04-10T19:53:42.843

i would write:

(defn chord [scale degree num_voices]
  (let [degree (if (keyword? degree) (.indexOf scale degree) degree)]
    (take num_voices (take-nth 2 (cycle (invert scale degree)))))

not sure it helps - no general principle, except using let. also, maybe others wouldn't like the way i shadow values with degree, but here i think it helps show the intent.

edit: compared to other answers, i have pulled out the value. i prefer this to embedding because i find a long chain of embedded evaluations harder to read. ymmv.

ps thinking some more [this some days later] if you are using this style in multiple places (where a parameter can be either a value or a key that pulls data from a preceding value) then i might consider writing a macro to automate that process (ie something that generates a fn with auto-generated let of the form above). the main problem is deciding how to indicate which paraneters are treated in that way (and also, i would worry about how this could confuse any ide you are using).

score 4 · Answer 3 · edited Apr 09 '12 at 21:40

In Clojure (and most other lisps) if returns a value just like every other expression. For example,

(if (even? 3) 1 0)

evaluates to 0.

You can use this knowledge to refactor your code by moving the identical portions of the code outside of the if statement, like so:

(defn chord [scale degree num-voices]
  (take num-voices (take-nth 2
                             (cycle (invert scale 
                                            (if (keyword? degree)  
                                                (.indexOf scale degree)
                                                degree))))))

Also, in Lisp, - isn't special or reserved, so you can and should use it in your variable names. It is better lisp style to use num-voices instead of num_voices or numVoices, as the dashed option is seen as more readable.

score 0 · Answer 4 · answered Apr 08 '12 at 17:03

There isn't much that can be done to simplify the procedure, maybe move the if inside the call to take num_voices, like this:

(defn chord ([scale degree num_voices]
   (take num_voices
         (take-nth 2
                   (cycle (invert
                           scale
                           (if (keyword? degree) (.indexOf scale degree) degree)))))))

clojure: idiomatic way of removing duplication in an "if"?

4 Answers4