Kids bank account in Clojure

Question

I'm learning clojure. A while back I made a python bank account for my kids, teaching them to save. Have a look here.

The Python one was a class and it created a table that had a little interest function that showed what their money will do if it sits there. Needless to say that teenagers are not impressed even by 15% annual interest. They want cash and now.

I am now implementing the same in Clojure. I haven't added interest yet. So first I'm asking for suggestion of how to do that, second thing will be any criticism of this approach. My logic might be off, yet the functions do work. The functions are: balance, transaction, report (with pprint table).

Namespaces:

(ns kids-bank.core
  (:require [clojure.string :as cstr])
  (:import java.util.Date)
  (:import java.io.File)   
  (:require [clojure-csv.core :as csv])
  (:require [java-time :as jt])
  (:require [clojure.pprint :as pp]))  


(def now jt/local-date)

Copying snippets from github the java.io seem to work only with use even when it is imported in ns.

(use 'clojure.java.io)

Creating accounts as csv files.

(defn create-account
  "starts a new csv file using name of holder in the designated folder
  using clojure.java.io checks if file is there to prevent erasing 
existing file"
  [name]
  (let [file-name (str "path/to/folder" name "-account.csv")]
    (if (.exists (file file-name))
      (println "Account already exists")  
      (do
        (.createNewFile (file file-name))
        (with-open [wrtr (writer (file file-name))]
          (.write wrtr "date,amount,reference \n")) (println "That's done for you!"))))) 

Creating the accounts:

(do (create-account "noah")
    (create-account "robin")
    (create-account "bea"))

(def robin-account "path/to/robin-account.csv")
(def bea-account "/path/to/bea-account.csv")
(def noah-account "/path/to/noah-account.csv")

Transaction

(defn transaction ;TODO add an optional arg for date, default today.
  "accepts string for file name, amount, and reference as a string
  and adds the transaction to the account"
  [file n ref]
  (with-open [wrtr (writer file :append true)]
    (.write wrtr (str (local-date (now)) ", " n  ", " ref "\n"))))

Parse the CSV

I modified some parsing csv function from @mobyte in order to have a default argument and the option to parse with or without the heading.

(defn take-csv
  "takes file name and reads data.
  by default skips the first header
  change key :header to true in order to include it"
  [& {:keys [fname header] :or {header false}}]
  (if (= header false)
    (rest (with-open [file (reader fname)]
            (doall (map (comp first csv/parse-csv) (line-seq file)))))
    (with-open [file (reader fname)]
      (doall (map (comp first csv/parse-csv) (line-seq file))))))

Balance

I needed to use Double from Java in a map function and found out that I need to define a Clojure function to use as fn argument in map, therefore little dble pre-code (is that the way to go about it?) .

(defn dble [a]
  "takes a string returns a double"
  (Double. a))

and:

(defn balance
  "accepts string for a file name returns the balance"
  [file]
  (reduce + (map dble (map second (take-csv :fname file :header false)))))

and finally the report

Printed in the repl as table:

(use 'clojure.pprint)

The use seems to be necessary in this case again (appreciate a clarification if someone has one).

(defn pretty-report
  "returns a table of history of account"
  [fname]
  (let [header (first (take-csv :fname fname :header true))]
    (loop [body (take-csv :fname fname :header false) result []]
          (if (empty? body)
            (do (println (str fname)) (pp/print-table result) (println 
(str "the balance is: " (balance fname))))
        (recur
         (rest body)
         (conj result (zipmap (map keyword header) (first body))))))))

results

Everything seem to work, here is an example:

kids-bank.core> (create-account "test")
That's done for you!
kids-bank.core> (def test-account "path/to/test-account.csv")
#'kids-bank.core/test-account
kids-bank.core> (transaction test-account 10.0 "allowence")
nil
kids-bank.core> (transaction test-account 5.0 "repayment")
nil
kids-bank.core> (transaction test-account 20.0 "from grandad")
nil
kids-bank.core> (transaction test-account -13.0 "shopping")
nil
kids-bank.core> (balance test-account)
22.0
kids-bank.core> (pretty-report test-account)
/path/to/test-account.csv

|      :date | :amount |   :reference  |
|------------+---------+---------------|
| 2018-05-24 |    10.0 |     allowence |
| 2018-05-24 |     5.0 |     repayment |
| 2018-05-24 |    20.0 |  from grandad |
| 2018-05-24 |   -13.0 |      shopping |
the balance is: 22.0
nil

Answer 1

This isn't bad code. The worst thing I see is a lot of your code is operating via side effects.

You're either directly reading from file with take-csv, or carrying out side-effects as the purpose of the function, like with transaction or pretty-report. All your functions are doing a little bit too much; I'd break all them all down a little more. You have to use side-effects somewhere, just make sure you're passing/returning data explicitly whenever feasible, then use those data-manipulating functions to help carry out the effects. This way you can test the processing of the data separate from, say, writing to disk.

I'll also just make a few nitpicky points:

Your imports are unnecessarily verbose. You can condense all the :requires and :imports together.

(ns kids-bank.core
  (:require [clojure.string :as cstr]
            [clojure-csv.core :as csv]
            [java-time :as jt]
            [clojure.pprint :as pp])
  (:import [java.util.Date]
           [java.io.File]))

---

(doall (map (comp first csv/parse-csv) (line-seq file)))

is arguably written oddly. I'd just use mapv here to do away with the need for doall. If you want laziness, use map, which returns a lazy sequence. If you want to evaluate everything strictly however, mapv returns a vector.

(mapv (comp first csv/parse-csv) (line-seq file))

---

For (Double. a), I personally prefer

(Double/parseDouble a)

I find it to be a little more explicit. It's exactly the same though, as the constructor just delegates to parseDouble anyway.

---

The method of summing generally regarded as more idiomatic is

(apply + nums)

instead of using reduce. This works because + has an overload that's a manual reduction over the arguments. They're basically the same though.

In that same area though, I think your double map could be cleaned up with comp, and/or ->>.

 (reduce + (map (comp dble second) (take-csv :fname file :header false)))

Or, what I personally prefer

(->> (take-csv :fname file :header false)
     (map second)
     (map dble)
     (apply +))

Which gives you a nice top-down read.

---

Really, you shouldn't shove multiple loop accumulators on a single line. This is like declaring multiple variables on the same line in other languages. It's a practice that's generally considered to hurt readability, and I think that's definitely the case here. Change

(loop [body (take-csv :fname fname :header false) result []]

to

(loop [body (take-csv :fname fname :header false)
       result []]

In that same part though, you keep calling first and rest on body. Just deconstruct body from the start. I'm also going to spread out your print calls as you're trying to put too much on one line again, and switch to when to get rid of the need for the do. Note, x and xs are terrible names, but I'm not sure in this case what to call them. I'm not advocating for single letter names here.

(defn pretty-report
  "returns a table of history of account"
  [fname]
  (let [header (first (take-csv :fname fname :header true))]
    (loop [[x & xs] (take-csv :fname fname :header false)
           result []]
      (when x
        (println (str fname))
        (pp/print-table result)
        (println (str "the balance is: " (balance fname)))
        (recur xs
          (conj result (zipmap (map keyword header) x)))))))

Note that (when x works because x will default to nil when the sequence is empty.

This whole part is just a reduction over what take-csv returns though. It could be written as

(defn pretty-report
  "returns a table of history of account"
  [fname]
  (let [header (first (take-csv :fname fname :header true))]
    (reduce (fn [result x]
              (println (str fname))
              (pp/print-table result)
              (println (str "the balance is: " (balance fname)))

              (conj result (zipmap (map keyword header) x)))
            [] ; Starting result
            (take-csv :fname fname :header false))))

---

The ... :or {header false}} part in take-csv is unnecessary. header will already default to nil via deconstruction if it isn't supplied, and that's falsey.

---

Best not to use the name ref. That's a core function already, and errors due to shadowing suck.

---

(if (= header false) ...)

isn't ideal. I'd either use

(if-not header ...)

or just reverse the order of the bodies and use

(if header ...)