4
\$\begingroup\$

I wrote Clojure code which takes named params and has to generate 2 .csv files as output.

Please review it.

(ns my_cli_clojure.core
  (:gen-class :main true))

(require '[clojure.data.csv :as csv]
     '[clojure.java.io :as io]
     '[me.raynes.fs :as fs]
     '[clojure.tools.cli :refer [cli] :as c])

(defn write_csv_data [& {:keys [data outfile]
                     :or {data ""
                          outfile ""}}]
  (csv/write-csv outfile
                data))

(defn -main
  [& args]
    (let [[options args banner]
  (c/cli args
 ["-d" "-dome" "Dome Identifier" :default "dome1"]
 ["-p" "-principal" "Principal code" :default "kraft"]
 ["-o" "-output-directory" "Output file" :default "."]
 ["-h" "-help" "Show Help" :flag true :default false])]

(when (:help options)
  (println banner)
  (System/exit 0))

 (let [output_directory (:output-directory options)]
(if-not (fs/exists? output_directory)
  (fs/mkdirs output_directory))
(doseq [n (clojure.string/split (:dome options) #",")]
  (let [dome_identifier (first (clojure.string/split n #"-"))]
(with-open [out-file (io/writer (clojure.string/join "/" [output_directory
                                                                  (clojure.string/join "." [dome_identifier "csv"])]))]
(write_csv_data :data [["dome_identifier" "name"]
                       [dome_identifier (first (reverse     (clojure.string/split n #"-")))]] :outfile out-file))

(with-open [dome_principal_file (io/writer (clojure.string/join "/" [output_directory
                                                                           (clojure.string/join "." ["dome_principal" dome_identifier "csv"])]))]
 (write_csv_data :data [(clojure.string/split "dome_identifier principal_code currency_code active latitude longitude" #"\s")
                        [dome_identifier (:principal options) "" "t" "" ""]] :outfile dome_principal_file)))))))
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Everything looked pretty good in general, until I got to the (let [output_directory ... part, then your indentation got all wonky! Part of the problem is that there are a couple of really long lines, and it's difficult to keep them short when you're nested so many layers deep into an S-expression. Shortening your lines and making sure that everything lines up from line-to-line in an intuitive way will solve this.

First of all, I believe it is idiomatic to include all your use/require/import statements within the namespace definition, like so:

(ns my_cli_clojure.core
  (:require [clojure.data.csv :as csv]
            [clojure.java.io :as io]
            [me.raynes.fs :as fs]
            [clojure.tools.cli :refer [cli] :as c]
            [clojure.string :as s])
  (:gen-class :main true))

Notice that I added [clojure.string :as s] -- this abbrevation will come in handy in shortening some of the later lines, in which you have a lot of fully-qualified references to clojure.string methods.

Next, just an aesthetic spacing tweak here:

(defn write-csv-data [& {:keys [data outfile] :or {data "", outfile ""}}]
  (csv/write-csv outfile data))

Your code was fine, I just find my version easier to read at a glance. I generally try to avoid using line-breaks whenever the code I'm writing looks sufficiently compact on less lines. When the lines start getting long, I will think more about where it would make the most sense to break them up. (I also renamed write_csv_data to write-csv-data; it's idiomatic in Clojure to name symbols using hyphens instead of underscores)

More spacing adjustments here:

(defn -main [& args]
  (let [[options args banner]
        (c/cli args
               ["-d" "-dome" "Dome Identifier" :default "dome1"]
               ["-p" "-principal" "Principal code" :default "kraft"]
               ["-o" "-output-directory" "Output file" :default "."]
               ["-h" "-help" "Show Help" :flag true :default false])]

Here is how I would tackle the part at the end:

(let [out-dir (:output-directory options)]
  (if-not (fs/exists? out-dir)
    (fs/mkdirs out-dir))
  (doseq [n (s/split (:dome options) #",")]
    (let [dome-id (first (s/split n #"-"))]
      (with-open [out-file (io/writer (str out-dir "/" dome-id ".csv"))]
        (write-csv-data :data [["dome_identifier" "name"]
                               [dome-id (last (s/split n #"-"))]] 
                        :outfile out-file))
      (with-open [dome-principal-file 
                  (io/writer (str out-dir "/dome_principal." dome-id ".csv"))]
        (write-csv-data :data [["dome_identifier" "principal_code" 
                                "currency_code" "active" "latitude" "longitude"]
                               [dome-id (:principal options) "" "t" "" ""]] 
                        :outfile dome-principal-file)))))))

Changes:

  1. I shortened a few symbol names, like output_directory => out-dir and dome_identifier => dome-id.
  2. I changed the two parts with the pattern like (s/join "/" [out-dir (s/join "." [dome-id "csv"])]) to (str out-dir "/" dome-id ".csv"), which is more concise and easier to read. Keep it simple!
  3. I simplified (first (reverse (s/split n #"-"))) to (last (s/split n #"-")).
  4. There's a part of your code where you created a list of strings by doing something like this: (s/split "first-string second-string third-string fourth-string" #"\s+"). That's clever, but the downside is that it leaves you with a long string that you can't break in order to shorten your lines. In this case, I think it makes more sense just to type e.g. ["first-string" "second-string" "third-string" "fourth-string"].
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.