Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The following code works, but I'd like to re-write, so that a single index is returned, rather than a sequence of indexes visited.

Here are the defs and function.

(def usage-vec-len 13)
(def read-levels [9999 3000 2000 1000 100 90 80 70 60 50 40 30 10])
(defn sel-adj-idx
"Finds the correct position at which to bin part of the reading."
[reading]
(for [idx (reverse (range 0 usage-vec-len))
     :let [out-idx idx]
     :when (and (<= reading (nth read-levels idx)) (>= idx 0))]
  out-idx))

If I run this with a reading of 5, I get what I expect

wtr-usage1.core=> (sel-adj-idx 5)
(12 11 10 9 8 7 6 5 4 3 2 1 0)
wtr-usage1.core=> (first (reverse (sel-adj-idx 5)))
0
wtr-usage1.core=> 

I keep going until the first index. The value of 5 would be binned there. I would like just to return the index, but still need to traverse read-levels to compare the reading against each of the levels.

Is there a better form to use for this purpose? I could probably write this recursively, but am trying to see if I can do it with Clojure's rich set of sequence functions.

share|improve this question
add comment

2 Answers

up vote 2 down vote accepted
  • Why not use just what you posted? Like this:

    ;use better names
    (defn sel-adj-idx-fixed [reading] (first (reverse (sel-adj-idx reading))))
    
  • for is used to generate sequences. Try loop for more general looping needs.

  • You should not hard code the length of a data structure. Each time you update read-levels you will need to update usage-vec-len. Use count instead. It is supposed to be fast for vectors.

  • A suggestion follows, Not a clojure programmer, so ignore the style:

    (defn bin-idx [reading]
       "......"
       (second (first (drop-while #(> reading (first %)) 
                                  (map vector 
                                       (reverse read-levels) 
                                       (range))))))
    
share|improve this answer
add comment

How about:

(def ^:const read-levels [10 30 40 50 60 70 80 90 100 1000 2000 3000 9999]

(defn bin-index [level]
  "Finds the correct position at which to bin part of the reading."                                                            
  (letfn [(find-idx [idx read-level]                                               
            (when (> read-level level) idx))]
    (or (first (keep-indexed find-idx read-levels))) (count read-levels)))

Steps:

  1. Find the first index where the read level is greater than our level.
  2. Give the last index (simply the number of levels we have) if our level is greater than all read levels.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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