Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've written an implementation of the back-propagation algorithm in Clojure (here). This is my first attempt at Clojure where the code totals more than ten lines and so it is not very idiomatic; specifically in one of my functions I have this:

; example neural-network: {:inputs [{:weights [1] :delta -0.2 :activation 1}] :outputs [{:activation 0.645}]} 
(defn train-layer
  [neural-network layer-index next-layer-index rate]
  (let [layer (get neural-network layer-index)
        next-layer (get neural-network next-layer-index)]
    (loop [neuron 0
           net neural-network]
      (if (< neuron (count layer))
        (recur (inc neuron) (loop [next-neuron 0
                                   net-1 net]
           (if (< next-neuron (count next-layer))
             (let [change (* (get-in next-layer [next-neuron :delta]) (get-in layer [neuron :activation]))
                   weight (+ (get-in layer [neuron :weights next-neuron]) (* rate change))]
               (recur (inc next-neuron) (assoc-in net-1 [layer-index neuron :weights next-neuron] weight)))

Where I need to change a neural network inside a nested for loop; making it an argument to each loop seems like a bad idea. It is also really rather dense (and therefore probably quite difficult to understand, so apologies). How should this be implemented to make it easier to read and more idiomatic?

share|improve this question
Refactor the inner loop into another function. Also, the loops could be turned into reducers. These would clean up the code considerably :) – Alex Nelson May 2 '14 at 3:54

Your Answer


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

Browse other questions tagged or ask your own question.