HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Generating two .csv files from named parameters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
csvnamedgeneratingfilestwofromparameters

Problem

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)))))))

Solution

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:

  • I shortened a few symbol names, like output_directory => out-dir and dome_identifier => dome-id.



  • 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!



  • I simplified (first (reverse (s/split n #"-"))) to (last (s/split n #"-")).



  • 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"].

Code Snippets

(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))
(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])]
(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)))))))

Context

StackExchange Code Review Q#41042, answer score: 4

Revisions (0)

No revisions yet.