From 9562705fdea598cf5fceb444c17e65659d58675c Mon Sep 17 00:00:00 2001
From: Yehonathan Sharvit <viebel@gmail.com>
Date: Fri, 2 Aug 2019 19:02:00 +0300
Subject: [PATCH] Update core.clj

---
 src/dove/core.clj | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/dove/core.clj b/src/dove/core.clj
index 1552b6f..dd1026a 100644
--- a/src/dove/core.clj
+++ b/src/dove/core.clj
@@ -187,6 +187,7 @@ Represents an amount of time defined by a number of months, days and
   `fixed(12)` in generated Java sources."
   (->avro-fixed? 12))
 
+;; CR: be careful with memoize. It might explode the memory!
 (def enum-obj-spec-value
   (memoize
     (fn [enum-class spec-values]
@@ -282,6 +283,7 @@ Represents an amount of time defined by a number of months, days and
         :optional
         :required))))
 
+;; CR: separate the code for each schema into its own expression 
 (extend-protocol ToSpec
   Schema$StringSchema
   (to-spec! [this args]
@@ -349,7 +351,7 @@ Represents an amount of time defined by a number of months, days and
         (do (spec-def args `~spec-keyword)
             (keyword (:spec-ns args) (:spec-name args)))
         (keyword (.getNamespace this) (.getName this)))))
-
+;; CR: create a function ->keyword that receives an avro obj and returns the corresponding keyword
   Schema$FixedSchema
   (to-spec! [this args]
     (let [spec-keyword (keyword (.getNamespace this)
@@ -363,6 +365,7 @@ Represents an amount of time defined by a number of months, days and
                                  :spec-ns (.getNamespace this)
                                  :spec-name (.getName this))
                                `~(->avro-fixed? (.getFixedSize this))))
+;; CR: this code appears several time - can you make a function of it?
       (if (and (:spec-ns args)
                (:spec-name args))
         (do (spec-def args `~spec-keyword)
@@ -407,6 +410,7 @@ Represents an amount of time defined by a number of months, days and
   Schema$RecordSchema
   (to-spec! [this args]
     (let [record-fields (.getFields this)
+          ;; CR: refactor to [spec-ns spec-name spec-keyword] (spec-data this): 3 lines combine into 1 :)
           spec-ns (.getNamespace this)
           spec-name (.getName this)
           spec-keyword (keyword spec-ns spec-name)
@@ -417,9 +421,11 @@ Represents an amount of time defined by a number of months, days and
                                 (update acc (optional-key? field args) conj spec-key)))
                             {}
                             record-fields)]
+      ;; CR: something is weird to-spec! seems to be a function that returns something but its return value is ignored
       (doseq [field record-fields]
         (to-spec! field (assoc args
                           :spec-ns (hierarchy-derive spec-ns spec-name))))
+      ;; CR: does spec-def have side effect? if yes rename to spec-def!
       (spec-def (assoc args
                   :spec-ns spec-ns
                   :spec-name spec-name)
@@ -430,6 +436,7 @@ Represents an amount of time defined by a number of months, days and
       (if (and (:spec-ns args)
                (:spec-name args))
         (do
+          ;; can you make spec-def return the keyword that corresponds to the spec. It will save the need for the `do` expression
           (spec-def args `~spec-keyword)
           (keyword (:spec-ns args) (:spec-name args)))
         spec-keyword))))