On Fri, 2011-12-30 at 15:21 -0800, rahulpilani wrote: > https://gist.github.com/1541958 > > I would appreciate any pointers on style or if I could implement it > any better.
Here are some style pointers on 74487e9, though they may be more specific than you were looking for. > 1: (ns prefix-tree) While this is just a sample, namespaces without at least one `.' are discouraged. I favor the Java convention (prefix with backwards Internet domain that you actually have), but I'm starting to see things like `slingshot.slingshot'. > 3: ;Record representing a node in the prefix tree. You'll find autoindentation easier if you follow something like the Emacs conventions for semicolon count: https://www.gnu.org/software/emacs/manual/html_node/elisp/Comment-Tips.html > 11: (let [a-length (.length a) b-length (.length b)] Despite its name, the builtin `count' function is better for this. > 15:(defn match-string [label string] You may find a simpler definition overall by taking advantage of the fact that `map' takes more than one sequence; start with `(map = label string)'. > 17: (defn mismatch-index [seq] Unlike Scheme, this won't make a lexical `mismatch-index'; it'll replace the current top-level binding for `mismatch-index'. So this makes `match-string' non-reentrant. > 24: (.charAt string index)))) seq))) Having `seq' here obfuscates its relationship with the rest of the expression. I favor: (filter (fn [index] (not (= (.charAt label index) (.charAt string index)))) seq) In other words, always starting the first argument to a function on the same line as the function, except as a last resort. As you saw later in the file, `->' and `->>' can help with indentation problems caused by heavy nesting. > 33: (let [substring (.substring string start end)] Try the builtin `subs'. > 34: (if-not (empty? substring) substring nil)))) Try `(not-empty substring)'. > 41: (hash-map :common (substring-or-nil label 0 match) :label-substring > (substring-or-nil label match) :string-substring (substring-or-nil string > match)))) Unlike other Lisps with literal vector/hash syntax, Clojure evaluates the contents, making these syntaxes not so "literal" in Clojure. So {:common ...} is better here. > 47: (defn first-char [string] > 48: "Return the first character of a string." > 49: (.charAt string 0)) This is `first', except that it doesn't do the same thing for empty strings. > 63: (assoc node :edges (apply assoc (.edges node) (edge-entry label > child))) :edges is better than .edges here. Accordingly, the builtin `update-in' can simplify this. > 70: (Node. (conj (.strings node) string) edges))) It remains to be seen, but it seems likely to me that the new (1.3) `map->Node' and `->Node' will become idiomatic here. -- Stephen Compall ^aCollection allSatisfy: [:each|aCondition]: less is better -- You received this message because you are subscribed to the Google Groups "Clojure" group. To post to this group, send email to clojure@googlegroups.com Note that posts from new members are moderated - please be patient with your first post. To unsubscribe from this group, send email to clojure+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/clojure?hl=en
