On Tue, Mar 3, 2009 at 6:40 AM, David Sletten <da...@bosatsu.net> wrote:
>
> I'd appreciate any feedback regarding my Clojure style. Any more
> natural ways to do things?
Looks good, thanks for sharing.
> (defn roman? [roman-string]
> (and (not (empty? roman-string))
> (re-matches
> #"(?:M{0,3})(?:D?C{0,3}|C[DM])(?:L?X{0,3}|X[LC])(?:V?I{0,3}|I
> [VX])$"
> roman-string)))
The normal idiom in Clojure is (seq x) instead of (not (empty? x)),
and that works fine for Strings. Also, since you don't really need
the specific return value of the test expressions, 'when' might be a
better choice than 'and'.
> (defn roman-to-arabic-aux [roman-string]
> (cond (empty? roman-string) 0
> (empty? (rest roman-string)) (value (first roman-string))
> (< (value (first roman-string)) (value (second roman-string)))
> (- (roman-to-arabic-aux (rest roman-string))
> (value (first roman-string)))
> :else
> (+ (value (first roman-string))
> (roman-to-arabic-aux (rest roman-string)))))
To reduce the clutter here, you might consider using destructuring on
roman-string to get locals for the first and rest parts:
user=> (let [[a & b] "fie"] [a b])
[\f (\i \e)]
You also might consider indenting the result expressions to help them
stand out more. I like either:
(cond
test1 (expr-fn
arg1 arg2)
test2 (expr-fn2
argA argB))
or:
(cond
test1
(expr-fn arg1 arg2)
test2
(expr-fn2 argA argB))
> (def arabic-values '((1000 "M") (900 "CM") (500 "D") (400 "CD")
> (100 "C") (90 "XC") (50 "L") (40 "XL")
> (10 "X") (9 "IX") (5 "V") (4 "IV") (1 "I")))
You might consider using a vector of vectors here. There's nothing
particularly wrong with what you've got, but I've been noticing
recently that almost every list you expressed directly in idiomatic
Clojure code is a function or macro call. I think this is part of why
I've found Clojure code to be less difficult to read than CL.
It doesn't matter much in this case as the quote is clearly visible
and the literal numbers aren't likely to be confused with function
calls, but I thought I'd mention it anyway.
> (defn arabic-to-roman
> ([n] (if (<= 1 n 3999)
> (apply str (arabic-to-roman n arabic-values))
> (format "%d cannot be converted." n)))
> ([n num-list] (cond (empty? num-list) '()
> (zero? n) '()
> :else (let [[[arabic roman] & tail] num-list]
> (if (>= n arabic)
> (cons roman (arabic-to-roman (- n
> arabic)
> num-list))
> (arabic-to-roman n tail)))) ))
You might consider using 'reduce' instead of recursion here.
Alternatively, it's interesting to note that because of the ease with
which you're destructuring arabic-values, it would be no more
difficult if you had a single list (or vector) of alternating numbers
and strings rather than nesting them.
--Chouser
--~--~---------~--~----~------------~-------~--~----~
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
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
-~----------~----~----~----~------~----~------~--~---
