CODEWITHSUNDEEP
clojure
kostas
kostas

Reputation: 2019

Clojure Inconsistent results in clojure during executions of a function

Clojure question

I have written the following function in clojure: In the first loop it iterates a list of maps and creates a map. Then the second loop iterates a list, matches data from the map previously created and a vector and returns a new map. However, different runs with the same data produce diffenet results. See below.

(defn resolve-case-per-period
  "Constructs a map by matching data existing in input parameter vectors"
  [dd case-details periods]
  (let [cases ((fn [in] 
                 (loop [case-details in, result-map {}] 
                   (if (= (count case-details) 0) 
                     result-map 
                     (recur (rest case-details) 
                            (assoc result-map
                              (:priority (first case-details)) 
                              (:caseid (first case-details)))))))
               case-details)
         periods periods]
    (info "Mapping cases to periods step 1 completed " cases)
    (loop [periods periods, result-map {}]
      (if (= (count periods) 0)
        result-map
        (recur (rest periods) 
               (conj result-map
                     { (str (:period (first periods)))
                       (get cases (:priority (first periods)))}))))))

The returned output is of a map like the following:

{31-10-10 20 10020101030122036M, 31-10-10 10 10020101030122036M, 31-10-10 21 10020101030122036M, 30-10-10 21 10020101030200157M, 31-10-10 00 10020101030122036M, 31-10-10 11 10020101030122036M, 31-10-10 22 10020101031112152M, 30-10-10 22 10020101030122036M, 31-10-10 01 10020101030122036M, 31-10-10 12 10020101030122036M, 30-10-10 23 10020101030122036M, 31-10-10 02 10020101030122036M, 31-10-10 13 10020101030122036M, 31-10-10 03 10020101030122036M, 31-10-10 14 10020101030122036M, 31-10-10 04 10020101030122036M, 31-10-10 15 10020101030122036M, 31-10-10 05 10020101030122036M, 31-10-10 16 10020101030122036M, 31-10-10 06 10020101030122036M, 31-10-10 17 10020101030122036M, 31-10-10 07 10020101030122036M, 31-10-10 18 10020101030122036M, 31-10-10 08 10020101030122036M, 31-10-10 19 10020101030122036M, 31-10-10 09 10020101030122036M}

Executing the function with the same parameters sometimes yields

{31-10-10 20 nil, 31-10-10 10 nil, 31-10-10 21 nil, 30-10-10 21 nil, 31-10-10 00 nil, 31-10-10 11 nil, 31-10-10 22 nil, 30-10-10 22 nil, 31-10-10 01 nil, 31-10-10 12 nil, 30-10-10 23 nil, 31-10-10 02 nil, 31-10-10 13 nil, 31-10-10 03 nil, 31-10-10 14 nil, 31-10-10 04 nil, 31-10-10 15 nil, 31-10-10 05 nil, 31-10-10 16 nil, 31-10-10 06 nil, 31-10-10 17 nil, 31-10-10 07 nil, 31-10-10 18 nil, 31-10-10 08 nil, 31-10-10 19 nil, 31-10-10 09 nil}

Upvotes: 0

Views: 128

Answers (2)

amalloy
amalloy

Reputation: 92147

Everything in this function is deterministic and pure (except the info calls, which shouldn't matter), so it should return the same thing every time. You don't provide any sample inputs, so I can't disprove this assumption.

The code is hard to read, and with no context I don't really understand what you're doing. But I went through your code in several refactoring passes to try to make it clearer what's going on. Hopefully this will help someone else who is reading, or even make it clearer to you where your problem is.

First

Remove all the crazy formatting and pointless variable-copying, and use seq instead of testing count=0

(defn resolve-case-per-period
  "Constructs a map by matching data existing in input parameter vectors"
  [dd case-details periods]
  (let [cases (loop [case-details case-details, result-map {}] 
                (if (seq case-details)
                  (recur (rest case-details)
                         (assoc result-map 
                           (:priority (first case-details))
                           (:caseid (first case-details)))) 
                  result-map))]
    (info "Mapping cases to periods step 1 completed " cases)
    (loop [periods periods, result-map {}]
      (if (seq periods)
        (recur (rest periods) 
               (assoc result-map
                 (str (:period (first periods)))
                 (get cases (:priority (first periods)))))
        (do (info "Mapping cases to periods step 2 completed " result-map)
            result-map)))))

Second

Destructuring and if-let instead of primitive keyword lookups, ifs, and seqs:

(defn resolve-case-per-period
  "Constructs a map by matching data existing in input parameter vectors"
  [dd case-details periods]
  (let [cases (loop [case-details case-details, result-map {}] 
                (if-let [[{:keys [priority case-id]} & more] (seq case-details)]
                  (recur more
                         (assoc result-map priority caseid)) 
                  result-map))]
    (info "Mapping cases to periods step 1 completed " cases)
    (loop [periods periods, result-map {}]
      (if-let [[{:keys [period priority]} & more] (seq periods)]
        (recur more
               (assoc result-map
                 (str period)
                 (get cases priority)))
        (do (info "Mapping cases to periods step 2 completed " result-map)
            result-map)))))

Third

At this point it's finally clear that we're just iterating over a sequence and building up a result value as we go, so we can drop all the first/rest nonsense and just use reduce to traverse the sequence for us:

(defn resolve-case-per-period
  "Constructs a map by matching data existing in input parameter vectors"
  [dd case-details periods]
  (let [cases (reduce (fn [result-map {:keys [priority case-id]}]
                        (assoc result-map priority caseid))
                      {}, case-details)]
    (info "Mapping cases to periods step 1 completed " cases)
    (reduce (fn [result-map {:keys [period priority]}]
              (assoc result-map
                (str period)
                (get cases priority)))
            {}, periods)))

Upvotes: 7

Paul
Paul

Reputation: 8182

It will be difficult to answer your question if we don't know what your data (function input) looks like, but a few points:

  1. dd is never used in your function, so you can get rid of that.
  2. It is idiomatic in Clojure to have fairly short functions, so I would suggest factoring the part that you do in let out into another function. That will also make testing and experimentation at the repl easier.
  3. Remapping periods to periods in the let has no effect, so get rid of that.
  4. You shadow a lot of locals (case-details in the let and periods in the loop) , this can be confusing and I would advise against it.
  5. The keys in your resulting map are strings, I presume of the form "31-10-10 20". This is easier to discern with the quotes.

I honestly don't see how this Clojure function could be responsible for giving you different outputs, are you absolutely sure that the input is identical? As an observation in the first case you get BigDecimals for the values, so my guess is that in the second case something that couldn't handle BigDecimals was in contact with the data. But I don't see how this could have happend in the function you provide.

Upvotes: 1

Related Questions