Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

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.

like image 918
Paul Sanwald Avatar asked Apr 08 '12 16:04

Paul Sanwald


1 Answers

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).

like image 80
andrew cooke Avatar answered Nov 03 '22 14:11

andrew cooke