Can I improve this code

Community Center Beginners
#1

Hello,

I did some experiments to make a web-app in the future.
Can I improve this one

(ns paintings.core
  (:require [cheshire.core :as json]))

(def slurp-memo (memoize slurp))

(defn read-all-data []
  (json/parse-string
   (slurp-memo "https://www.rijksmuseum.nl/api/nl/collection?key=14OGzuak&format=json&type=schilderij&toppieces=True")
   true))

(defn filter-artObjects [data]
  (get data :artObjects))

(defn filter-all-objectNumbers [data]
  (map :objectNumber data))

(defn ask-for-image-data [objectNumbers]
  (for [number objectNumbers]
    (-> (json/parse-string
         (slurp-memo (str "https://www.rijksmuseum.nl/api/nl/collection/" number "/tiles?key=14OGzuak&format=json"))
         true)
        (assoc :object-number number))))

(defn extract-url-and-size [image]
  (let [data (select-keys image [:width :height])
        url  (get-in image [:tiles 0 :url])]
    (assoc data :url url)))

(defn filter-image-url [imagedata]
  (-> imagedata
      :levels
      (as-> levels (sort-by :name levels))
      (last)
      (extract-url-and-size)
      (assoc :object-number (get imagedata :object-number))))


  (->> (read-all-data)
       (filter-artObjects)
       (filter-all-objectNumbers)
       (ask-for-image-data)
       (map filter-image-url))
#2

Instead of memoizing slurp, you could have a look at core.cache, it could be a useful tool for other things as well.

Wrt the functions, the naming might be better (filter-all-objectNumbers does no filtering at all, and arguably, could just be a lambda in the last pipeline…)

#3

it doesn’t
As far as I see it , it takes care that al the objectNumbers are taken out of the json.
So for me that is filtering

I will look at the cache part.
And what do you mean with could be a lambda

#4

But that is not filtering, in FP parlance (and so the name will be misleading in a Clojure codebase). Filtering you do with filter and related functions, and those generally will take things out of a collection, not select keys from a map.

Re: the lambda, is that a one-liner function that is self descriptive could just be inlined in (fn..) form. It’s good to name things, in general, but in many cases, lambdas are more idiomatic.

#5

oke, so I can better name it select-all-objectNumbers or just select-ObjectNumbers

what is then a better name for this one (defn ask-for-image-data [objectNumbers]
here I go to the api and request for the image data

#6

Maybe “request-data” or “fetch-data”…

If you’d like to use this project to learn some libraries and how you’d do this “properly” (for some value of “proper”), you might want to look in to http-kit or clj-http, for making your requests (rather than manually building the URL and slurping it), core.cache for the caching (though ATM I don’t see why you’d be caching this, since you basically request everything only once)

Also, what I mentioned about the lambdas, you could write something like this when you’re prototyping this at the REPL:

(->>  "https://www.rijksmuseum.nl/api/nl/collection?key=14OGzuak&format=json&type=schilderij&toppieces=True"
      slurp
      json/read-str
      clojure.walk/keywordize-keys
      :artObjects
      (map :objectNumber)
      ask-for-image-data
      (map filter-image-url))

Note that the intent remains very clear at each step, and you don’t necessarily need to defn the one-liners.

#7

I allowed myself to change a little bit your code.

(ns paintings.core
  (:require [cheshire.core :as json]))

(def slurp-memo (memoize slurp))

(defn parse [url]
  (json/parse-string (slurp-memo url) true))

(defn image-url-size [image]
  (let [data (select-keys image [:width :height])
        url (get-in image [:tiles 0 :url])]
    (assoc data :url url)))

(defn assoc-image [object-number]
  (->> (str "https://www.rijksmuseum.nl/api/nl/collection/" object-number "/tiles?key=14OGzuak&format=json")
       (parse)
       (:levels)
       (sort-by :name)
       (last)
       (image-url-size)
       (merge {:object-number object-number})
       ))

(->> "https://www.rijksmuseum.nl/api/nl/collection?key=14OGzuak&format=json&type=schilderij&toppieces=True"
         (parse)
         (:artObjects)
         (map :objectNumber)
         (map assoc-image)
         )

Your filters functions are simply wrapping maps methods, you can unwrap them and use directly in your thread-macro.
The final result by the way is really similar but, instead of retrieving all object numbers and iterate each one to search the correct image to associate, i simply handle this operation within the object-number image retrieval.
Hope you’ll find this useful.

3 Likes
#8

of cpurse I want to learn things “properly”

Caching could be working on another level. This will be a multipage website and maybe it can be wise to “cache” the image fetching part so the next time the same page is asked and the same images has to be shown it coming from the “cache” instead of every time asking the api for all the data.

#9

Thanks for showing me.
This could be working but I have to see how I can make another call where I fetch some data later if a user wants to see that data.

You code is much much cleaner then I have now.

#10

I tried but get a lot of errors so far

(ns paintings.core
  (:require [cheshire.core :as json]
            [clj-http.client :as client]))

(def slurp-memo (memoize slurp))

(defn image-url-size [image]
  (let [data (select-keys image [:width :height])
        url (get-in image [:tiles 0 :url])]
    (assoc data :url url)))

(defn assoc-image [object-number]
  (->> (json/parse-string (client/get
                           (str "https://www.rijksmuseum.nl/api/nl/collection/" object-number "/tiles")
                           {:query-params {:key "14OGzuak"
                                           :format "json"}}))
       (:levels)
       (sort-by :name)
       (last)
       (image-url-size)
       (merge {:object-number object-number})))

(time (->> (json/parse-string (client/get "https://www.rijksmuseum.nl/api/nl/collection"
                                          {:query-params {:key "14OGzuak"
                                                          :format "json"
                                                          :type "schilderij"
                                                          :toppieces "True"}}))
           (:artObjects)
           (map :objectNumber)
           (map assoc-image)))
#11

You probably need to extract the :body of the response before trying to access its contents.
You should get in the habit of exploring the results you get from the REPL.

I’m not sure how is it with Calva, but in CIDER there’s a very useful command that evaluates an expression and pretty-prints the results, which is very handy for this type of stuff.

#12

got it working

  (:require [cheshire.core :as json]
            [clj-http.client :as client]
            [clojure.walk :as walk]))


(defn image-url-size [image]
  (let [data (select-keys image [:width :height])
        url (get-in image [:tiles 0 :url])]
    (assoc data :url url)))

(defn assoc-image [object-number]
  (->> (client/get (str "https://www.rijksmuseum.nl/api/nl/collection/" object-number "/tiles")
                           {:query-params {:key "14OGzuak"
                                           :format "json"}})
       (:body)
       (json/parse-string)
       (walk/keywordize-keys)
       (:levels)
       (sort-by :name)
       (last)
       (image-url-size)
       (merge {:object-number object-number})))

(assoc-image "SK-A-4830")

(time (->>(client/get "https://www.rijksmuseum.nl/api/nl/collection"
                                          {:query-params {:key "14OGzuak"
                                                          :format "json"
                                                          :type "schilderij"
                                                          :toppieces "True"}})
           (:body)
           (json/parse-string)
           (walk/keywordize-keys)
           (:artObjects)
           (map :objectNumber)
           (map assoc-image)))