Read Space Delimited Text File to Standardized Data Type

Question

From the README.md

Introduction:

The purpose of data-in is to read all the lines from a text file and return the file's contents as a Clojure value in a standardized manner.

Specification:

Takes:

• Filename

Assumes:

• a space delimited text file

Returns:

• Vector [L₁,L₂,...L_n] where L_i:
  • represents a line in Filename
  • is a vector
  • contains EDN values

Example:

If my-file.txt contains:

3 9
5 4
6 5
3 2

then (data-in "./my-file.txt") returns:

[[3 9] [5 4] [6 5] [3 2]]

Comments:

1. To avoid executing arbitrary code, data-in reads values from files with clojure.edn/read-string.

2. The input is not read lazily. The assumption being that sorting or an equivalent operation will be performed. The basis for this assumption is the proximate use case: reading data files for Discreet Optimization @ Coursera.

3. The private function f is called f to match the argument f at the call site in the private function get-data. This reflects a personal preference for mathematical notation when invoking mathematical abstractions.

Code:

(ns data-in.core
  (:require [clojure.java.io :as io]
            [clojure.edn :as edn]))


(defn- get-data
  [data-file f]
  (with-open [rdr (clojure.java.io/reader data-file)]
        (doall (map f (line-seq rdr)))))


(defn- f
  [line]
  (vec (map (fn [x] (edn/read-string x)) 
            (clojure.string/split line #" "))))

(defn data-in
  [filename]
  (vec (get-data filename f)))

Answer 1

First off, I'm going to have to complain about your taste in function names, which will probably be unproductive and just lead to hurt feelings on both sides. Yes, it's that function f. I don't mind so much if small inner functions defined with letfn have names like f, but to my eye, something declared at the top level of namespace—private or no—should have a more descriptive name. I would probably not define f as a top-level function at all; see below for more on that. It's also confusing that you call the top-level function f, then also call the function passed as a parameter to get-data by the name f and then go ahead and pass f as the parameter f in data-in.

I kind of don't like that you expect the output of data-in to be a vector of vectors, but you depend on the function passed as a parameter to get-data to return a vector instead of putting that logic inside data-in. If I had a top-level private function that's only used once and seems too specific to ever be useful again, like f, I would consider making it an anonymous function that gets passed in the one time it's used. So I would probably rewrite data-in like this:

(defn data-in
    [filename]
    (vec (get-data filename #(vec (map edn/read-string 
                                       (clojure.string/split % #" ")))

Or, if you're uncomfortable with the shortcut syntax for anonymous functions (it can get pretty hairy), like this:

(defn data-in
    [filename]
    (vec (get-data filename (fn [line] (vec (map edn/read-string 
                                             (clojure.string/split line #" ")))

Now data-in is longer and a bit uglier. But this way, all the logic for data-in is in data-in. You expected data-in to return a vector of vectors, but as it was before, data-in relied on f processing each line into a vector, so the logic of one function was spread across multiple functions.

Notice how the whole thing is now separated into a general portion, the get-data function, which could be useful again later on when you need to read some lines from a file, process them in some way, and return them in a sequence; and a specific portion, data-in, that contains all that bits which are only relevant to the problem you have right now.

Pursuant to that, I don't really like that get-data is private. It seems like a perfectly good, general, widely useful function. For example, suppose we have a CSV file with columns a, b, and c, separated by spaces. Then we could write:

(get-data "MyCSV.csv" #(let [{a 0, b 1, c 2} (clojure.string/split % #" ")]
                            {:a a, :b b, :c c}))

and instead of a sequence of vectors, we'd get back a sequence of maps with keys :a, :b, and :c. This usage is totally unrelated to your original intention for this code, but it shows how generally useful the function is—so why make it private and hide it where no one can access it? Let it out and see what uses people can find for it.

Finally, for such a small program it probably doesn't matter, but one of my favorite features of Clojure is docstrings. I would write one for any function I was publicly exposing:

(defn data-in
    "Returns a vector of vectors of elements from the given file."
    [filename]
    (vec (get-data filename #(vec (map edn/read-string 
                                       (clojure.string/split % #" ")))

Then anyone can call up a REPL, load your namespace, and type (doc data-in) to read the documentation.