1
\$\begingroup\$

My first crack at clojure... if you have time to provide input, I'd appreciate it. I'm as much interested in style as proper usage. I'm just getting started.

The task to start with an vector of N zeros, and then "scramble" it by: Select 2 items in the vector. If one is less than 4, and the other is greater than -4, increment one and decrement the other -- so that the sum of the vector remains the same.

  • am I abusing let?

  • this generates a sequence of vectors, and I take the last one. Is there a better way to iterate and get the last result?

  • what tricks that I've missed?

  • this is just a small sample ;) but what community standards and practices have I missed?

code

(defn level [c] (let [n+ (rand-int (count c))
               n- (rand-int (count c))
               v+ (inc (nth c n+))
               v- (dec (nth c n-))
               go (and (not= n+ n-) (< v+ 4) (> v- -4))
               ]
           (if go (assoc (assoc c n+ v+) n- v-) c)
           ))

(defn rseq [n]
      (let [first (vec (repeat n 0))]
        (last (take (* n 2) (iterate level first)))))

(rseq 16)

(deftest rseq-sum-equals-2
  (let [actual (rseq 16)]
  (is (= 0 (apply + actual)))))
(run-tests 'ursound.core)
\$\endgroup\$
  • \$\begingroup\$ There's no code yet, you mean? \$\endgroup\$ – Jamal Mar 4 '14 at 4:52
  • \$\begingroup\$ lol hrm ok first question... how do I get stackexchange to not collapse my code? ;) \$\endgroup\$ – Rob Mar 4 '14 at 4:56
  • 1
    \$\begingroup\$ You have a couple of off-by-one errors. To comply with your description, go should be bound to (and (not= n+ n-) (<= v+ 4) (>= v- -4)). \$\endgroup\$ – Thumbnail Jun 6 '14 at 12:36
1
\$\begingroup\$

I am also relatively new to Clojure, but here are my first thoughts:

  • I like that you work with a vector, because random access of vectors with (nth) and assoc is faster. I also like your naming. thanks for providing a deftest, but please, user a proper inlining.

  • (assoc)'s do not need to be nested: (assoc (assoc c n+ v+) n- v-) is the same as (assoc c n+ v+ n- v-).

  • (last (take n x)) can be written as (nth x n).

  • Warning: you use the name 'first in yout (let), but this name is already defined in clojure.core.

  • For a style guide, please see https://github.com/bbatsov/clojure-style-guide

That is all for now, and I hope you found some advice useful.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Excellent! Those are all good points! Thank you for the reference to the style guide... I'm sort of drifting into clojure sideways ;) but right away I'm seeing lots of good tips in there. \$\endgroup\$ – Rob Mar 10 '14 at 22:59
  • 1
    \$\begingroup\$ @RobY ... and write (c x ) instead of (nth c x ). \$\endgroup\$ – Thumbnail Jun 6 '14 at 12:31
1
\$\begingroup\$

Sorry to necro-bump this, but there's a key thing I noticed about your code that hinders readablity.

You aren't "abusing" let, I think that would be difficult, but your bindings are definitely misaligned. Ideally, all of your bindings should line up. Many IDE's enforce/highly recommend this, and it has the added benefit of allowing tools to manage parentheses for you based on indentation (so code kind of resembles Python). In your case, your code would become:

(defn level [c] (let [n+ (rand-int (count c))
                      n- (rand-int (count c))
                      v+ (inc (nth c n+))
                      v- (dec (nth c n-))
                      go (and (not= n+ n-) (< v+ 4) (> v- -4))
           ]

One other thing I noticed is, contrary to most other "language families", in Lisps, braces are recommended to be on the same line instead of taking up a line to themselves.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.