4
votes

I am new to clojure and as an exercise in learning the language I am rewriting one of my old groovy scripts in clojure. For context, the script queries a JIRA instance for time entries, receives a result in json and generates a report based on the response.

I realize questions on traversal of nested structures have been asked ad infinitum on s.o., but I failed to find a direct answer to this so I am hoping for some help from clojurists on an idiomatic and concise way. The core problem is generic and not related to this particular piece of code.

I'm looking to rewrite the following in clojure:

// GROOVY CODE
// this part is just here for context
def timeFormat = DateTimeFormat.forPattern('yyyy/MM/dd')
def fromDate   = timeFormat.parseDateTime(opts.f)
def toDate     = timeFormat.parseDateTime(opts.t)

def json       = queryJiraForEntries(opts, http)
def timesheets = [:].withDefault { new TimeSheet() }

// this is what I'm hoping to find a better way for
json.issues.each { issue ->
  issue.changelog.histories.each { history ->
    def date = DateTime.parse(history.created)
    if (date < fromDate || date > toDate) return

    def timeItems = history.items.findAll { it.field == 'timespent' }
    if (!timeItems) return

    def consultant = history.author.displayName
    timeItems.each { item ->
      def from = (item.from ?: 0) as Integer
      def to   = (item.to   ?: 0) as Integer

      timesheets[consultant].entries << new TimeEntry(date: date, issueKey: issue.key, secondsSpent: to - from)
    }
  }
}

(sample structure of the returned json can be found here)

Note that when we create the resulting time entry, we use issue.key from the outermost level, date from the intermediate level, and from and to from the innermost level of the nested structure.

In groovy, a return within an each loop just exists the innermost each. I believe the rest of the code should be more or less self explanatory.

So the generic problem I am trying to solve is: given a deeply nested structure of maps and lists:

  • traverse/filter to a specific depth of the structure
  • perform some operation on the data at that level of depth and add result to context
  • traverse/filter deeper into the structure
  • perform some operation on the data at that level of depth and add result to context
  • ...
  • at some final level, produce a result based on the data in the context and the data available at that level.

I find this type of traversing with context and transforming data to be an increasingly common pattern.

My current solution is more verbose than the groovy one and for my untrained-at-reading-clojure-code eyes, much harder to understand at a glance. The details of parsing dates etc are unimportant. What I'm looking for is an concise clojure pattern for this.

edit 1: per request in comment, here is my current code. I apologize in advance and shamelessly blame it all on my total newbieness:

;; CLOJURE CODE
(defn valid-time-item? [item]
  (and (= (:field item) "timespent") (:to item) (:from item)))

(defn history->time-items [history]
  (filter valid-time-item? (:items history)))

(defn history-has-time-items? [history]
  (not-empty (history->time-items history)))

(defn history-in-date-range? [opts history]
  (tcore/within? (tcore/interval (:from-date opts) (:to-date opts))
                 (tformat/parse (tformat/formatters :date-time) (:created history))))

(defn valid-history? [opts h]
  (and (history-has-time-items? h) (history-in-date-range? opts h)))

(defn issue->histories-with-key [issue]
  (map #(assoc % :issue-key (:key issue))(get-in issue [:changelog :histories])))

(defn json->histories [opts json]
  (filter #(valid-history? opts %) (flatten (map issue->histories-with-key (:issues json)))))

(defn time-item->time-entry [item date issue-key]
  (let [get-int (fn [k] (Integer/parseInt (get item k 0)))]
    {:date          (tformat/unparse date-formatter date)
     :issue-key     issue-key
     :seconds-spent (- (get-int :to) (get-int :from)) }))

(defn history->time-entries [opts history]
  (let [date       (tformat/parse (tformat/formatters :date-time) (:created history))
        key        (:issue-key history)]
    (map #(time-item->time-entry % date key) (history->time-items history))))

(defn json->time-entries [opts json]
  (flatten (map #(history->time-entries opts %) (json->histories opts json))))

(defn generate-time-report [opts]
  (json->time-entries opts (query-jira->json opts)))

with some of the scaffolding etc omitted for brevity. The entry point in the above is generate-time-report which returns a collection of maps.

In issue->histories-with-key I retain the issue.key context by sticking the issue key into each history map. Aside from the general structure of the code, this is one of the points I find ugly and non-scalable. Also I haven't added the consultant dimension to the clojure solution yet.

edit 2: a second attempt after some fiddling and input from comments and the answer below. This one somewhat shorter, using a structure closer to the original code and has the consultant piece from the original code included:

;; CLOJURE CODE - ATTEMPT 2
(defn create-time-entry [item date consultant issue-key]
  (let [get-int #(Integer/parseInt (or (% item) "0"))]
    {:date          (f/unparse date-formatter date)
     :issue-key     issue-key
     :consultant    consultant
     :seconds-spent (- (get-int :to) (get-int :from)) }))

(defn history->time-entries [history issue-key from-date to-date]
  (let [date       (f/parse (f/formatters :date-time) (:created history))
        items      (filter #(= (:field %) "timespent") (:items history))
        consultant (get-in history [:author :displayName])]
    (when (and (t/within? (t/interval from-date to-date) date) (not-empty items))
      (map #(create-time-entry % date consultant issue-key) items))))

(defn issue->time-entries [issue from-date to-date]
  (mapcat #(history->time-entries % (:key issue) from-date to-date)
          (get-in issue [:changelog :histories])))

(defn json->time-entries [json from-date to-date]
  (mapcat #(issue->time-entries % from-date to-date) (:issues json)))

(defn generate-time-report [opts]
  (let [{:keys [from-date to-date]} opts]
    (filter not-empty
            (json->time-entries (query-jira->json opts) from-date to-date))))
2
Post what you have tried in Clojure code. - Frank C.
clojure attempt added to question - Matias Bjarland
-> is a macro in Clojure, so maybe don't use them in function names. Also, letfn can help you clean this up a bit. - jmargolisvt
I don't really have a preference here, but according to at least one clojure style guide, it seems that -> in conversion method names is a recommended practice. Opinions? - Matias Bjarland
Your code might be better expressed using declarative data description and validation, such as Plumatic/Schema provides. I'm not sure whether the new Clojure spec is as good a fit. - Thumbnail

2 Answers

2
votes

I think your Clojure code isn't bad at all. This is how I would improve it. Just a few changes.

(defn valid-time-item? [item]
  (and (= (:field item) "timespent") (:to item) (:from item)))

(defn history->time-items [history]
  (filter valid-time-item? (:items history)))

(defn history-has-time-items? [history]
  (not-empty (history->time-items history)))

(defn history-in-date-range? [history from-date to-date]
  (tcore/within? (tcore/interval from-date to-date)
                 (tformat/parse (tformat/formatters :date-time) (:created history))))

(defn valid-history? [h from-date to-date]
  (and (history-has-time-items? h) (history-in-date-range? h from-date to-date)))

(defn issue->histories-with-key [issue]
  (map #(assoc % :issue-key (:key issue)) (get-in issue [:changelog :histories])))

(defn time-item->time-entry [item date issue-key]
  (let [get-int (fn [k] (Integer/parseInt (get item k 0)))]
    {:date date
     :issue-key issue-key
     :seconds-spent (- (get-int :to) (get-int :from))}))

(defn history->time-entries [history]
  (map #(time-item->time-entry % (:created history) (:issue-key history)) (history->time-items history)))

(defn json->time-entries [json opts]
  (let [{:keys [from-date to-date]} opts]
    (->> json
         :issues
         (mapcat issue->histories-with-key)
         (filter #(valid-history? % from-date to-date))
         (mapcat #(history->time-entries %)))))

(defn generate-time-report [opts]
  (json->time-entries opts (query-jira->json opts)))

Key changes

  • Unnested json->time-entries implementation. Now it's much clear how json becomes time-entries. E.g. json -> issues -> history -> time-entry
  • Use mapcat instead of (flatten (map ...
  • Destructure :from-date and :to-date earlier. I think sending from-date and to-date into function makes function signature more readable than opts
  • Swap postion of subject (json) and opts. Put the most important argument at the first position except if it's collection function that takes in lambda like map, filter, etc.
1
votes

After two years I would like to post a suggested solution to my own question.

It seems that one way to solve the "descend to a certain depth, capture some context, descend further, capture some context" problem in clojure is for comprehensions.

Using for comprehensions you could do something like the following:

(defn clojure-rewrite [opts http]
  (let [from-date (local-date-time (:f opts) 0)
        to-date   (local-date-time (:t opts) 23 59)
        json      (query-jira opts http)]
    (for [issue   (-> json :issues)
          history (-> issue :changelog :histories)
          :let    [date (local-date-time df (:created history))]
          :when   (before? from-date date to-date)
          item    (:items history)
          :when   (= (:field item) "timespent")
          :let    [secs #(Integer/parseInt (or (% item) "0"))]]
      {:consultant    (-> history :author :displayName)
       :date          date
       :issue-key     (:key issue)
       :seconds-spent (- (secs :to) (secs :from))})))

to accomplish essentially what the original groovy code was doing. This method returns a collection of maps which is idiomatic in clojure.

The code uses java 8 and the excellent clojure.java-time library (require not included in the code for brevity) which makes the date parsing somewhat different but I believe the pattern is quite generally applicable.

I think this illustrates that it is possible to solve the original (and fairly generic) problem in clojure and stay concise. In fact even more concise than the groovy code which was my initial goal and hope.