current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I'm working on my first Clojure project, which is a parser. Since it's my first project I'm unsure of many aspects, from my general approach to solving this problem to the small implementation details. I also don't know anyone proficient in Clojure so any guidance is welcome.

In higher terms, my implementation will take a string (which is one message to be parsed) and break it into fields. Each field it will apply some function to (often a dictionary lookup), and finally return a dictionary of results.

(use 'clojure.string)

;; This defines the breakdown of each line in the file. The line contains an entry and start with something
;; like "1AXYZ ". In this case the first two characters ("1A") are the "Record Type" and the next four
;; ("XYZ ") are the "Entering Firm". This data structure holds that information.
(def parser
  {:shared '(  ; These are the entries shared amongst all types of message

          ;; Field-Name  num-chars
             Record-Type 2   
             Entering-Firm 4
             Clearing-Number 4
             Symbol 11
             Lot-Indicator 1
             ;;...
             )

   :1A     '(      ; this is one type of message, indicated with record-type=1A

          ;; Field-Name  num-chars
             Filler 9
             Poss-Dupe 1
             Booth-Booked-Indicator
             Order-Time 6
             Order-Date 8
             ;; ...
             )
   }
)


;; if no implementation is found for a field, just return a map to its contents.
(defn default [header x]
  {(str header) (trim (join x))})


;; most of the fields that dont use the default function will just be a dictionary lookup    
(defn Poss-Dupe [[x]] (default 'Poss-Dupe ({\1 "Odd Lot" \D "Rule 902" \E "Poss Dupe Rule 902" 
                                            \F "Auto Execution" \G "Poss Dupe of Auto-Execution" 
                                            \H "Non-auto Execution" \I "Poss Dupe of Non-auto Execution"
                                            \0 "" \space ""}
                                               x (str "ERROR: " x))))

(defn Booth-Booked-Indicator [[x]] (default 'Booth-Booked-Indicator ({\0 "Post" \1 "Booth" \2 "Booked"} x (str "ERROR: " x))))

(defn Order-Time [x] (default 'Order-Time (->> x (partition 2) (map join) (join ":")  ))) ; "HHMMSS" -> "HH:MM:SS"

(defn Order-Date [x] (default 'Order-Date 
                       (join "-" (map join (list (take 4 x) (take 2 (drop 4 x)) (drop 6 x)))))) ; "YYYYMMDD"->"YYYY-MM-DD"

(defn Filler [x] nil)


(defn try-call [f subseq]
  (try
    ((eval f) subseq)
    (catch Exception e  ; f not defined -> call default
      (default f subseq))))


;; Where the recursion happens
(defn grab-call [[f n & rest-instructions] line] 
  (let [[field rest-line] (split-at n line)]
    (if (or (empty? line) (not f)) 
      nil
      (merge (try-call f field) (grab-call rest-instructions rest-line)))))


;;; Entry point
(defn parse-line [line]
  (grab-call (concat (:shared parser) 
                     ((keyword (subs line 0 2)) parser)) 
    line ))

If you care to know the particulars of what is being parsed, that can be found at NYSE MRO specs.

share|improve this question

2 Answers 2

up vote 1 down vote

There is a lot of weird stuff here.

  • It's strange to have no ns form for defining your namespace and requiring the libraries you need. Putting a bare use at the top is gross.
  • Don't use an entire namespace. Either require with an alias, like (:require [clojure.string :as s]) ... (s/join ...), or else require and refer the specific stuff you use: `(:require [clojure.string :refer [join trim]]).
  • The uppercase function names should be lowercase: poss-dupe, or even possible-duplicate, not Poss-Dupe
  • There should be a newline after the parameter definitions for most functions: (defn foo [x] \newline (...))
  • The eval is very suspicious, but I don't want to read your whole program and figure out what you should be doing instead. Probably ns-resolve or something.
  • (if (or (empty? x) f) nil y) is a bad way to write (when (and (seq x) f) y)
  • Usually calling keyword to look stuff up in a map suggests that you shouldn't be using keywords as keys, but in your case it looks fine-ish. However, instead of ((keyword whatever) m), it would be more readable to write (get m (keyword whatever)).
share|improve this answer
    
Thanks for the tips! ns-resolve does exactly what I was using eval for--why is it preferred in general? And are you still (as you were in IRC) of the opinion that I should have an explicit map rather than just a function lookup? I was going for minimal boilerplate, but I don't want to sacrifice readability for that... –  ari Jun 9 at 20:46
    
Yes. Otherwise you have to give your functions crappy names like Poss-Dupe, and you run the risk of executing arbitrary code if someone puts an eval into their CSV file or whatever it is you're parsing. –  amalloy Jun 9 at 20:49
    
Haha, the field is called "Poss Dupe" in the original NYSE spec. –  ari Jun 9 at 21:33
    
Yes, exactly. Your scheme involving eval and ns-resolve means you have to give your functions names as crappy as those in the nyse spec, instead of functions with good names and a mapping from crappy names to good ones. –  amalloy Jun 9 at 21:33
    
Ahh, that's a good point –  ari Jun 9 at 21:35
up vote 0 down vote

Some points on idiomatic Clojure:

  • Do not return nil from if statements: (if condition? nil false-branch) -> (if-not condition? false-branch)
  • defn bodies in the next line unless they are truly one liners (not like in Booth-Booked-Indicator.
  • Don't use eval but resolve the function and call it directly as (f).

Conceptually:

  • If you want to define your own types use defrecord and deftype (defrecord in this case). Poss-Dupe, Order-Time Order-Date, etc. should be records, and it might be cleaner if they use the same fn bindings, instead of [[x]] and [x], only one of the two.
  • Instead of defining of passing around a function, evaluating it, checking for a NullPointerException, and then going to the default implementation, consider using a protocol. For example, IParse (parse [f]). You can then implement parse for your different types and leave a default implementation. Then try-call will be just reduced to -parse.
share|improve this answer
    
(eval f) -> (f) is a totally wrong suggestion. Getting rid of the eval is great, but you completely change what the function does. Records and protocols don't make a lot of sense to me here, but I guess I don't understand the problem enough to say for sure that it is wrong. –  amalloy Jun 9 at 18:06
    
I don't know what you mean by "call it directly as (f)"--the following dummy example gives an error: (def foo 'bar) (defn bar [] (println "bar")) (foo) –  ari Jun 9 at 20:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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