On Tue, Jan 18, 2011 at 12:37 PM, Alan <a...@malloys.org> wrote:
> defstruct is old and I'm pretty sure there's no reason to use it.
> defrecord is a drop-in replacement though for something simple and not
> performance-sensitive you might consider just using hashmaps.
Agree, but in a case like this there's no strong reason to change it
after-the-fact; at least none that I am aware of.
> to-resolution seems like it should be named get-resolution or
> resolution-of or something, but the implementation is fine.
Yes, I noticed a few function names I'd have made slightly different.
> wallpaper-resolution? has a common antipattern: #(= % blah) is usually
> better written as #{blah} because of the way sets behave when treated
> as functions.
I'm not sure I agree here. #(= % blah) may be a little longer but it's
very clear that it's an equality comparison; #{x y z} to check for
being any of some small set of alternatives seems fine, but #{x} seems
slightly obscure compared to #(= % x) and probably compiles to less
efficient bytecode (though that's not a major consideration most of
the time, including here in an obviously I/O-bound setting).
> to-wallpaper-name looks fine to me, though you would probably be
> happier using clojure.core/subs instead of String.substring.
Between defstruct and .substring I'd guess he's either using a version
older than 1.2, or cut his teeth on an older version and isn't
completely familiar with the new stuff in 1.2 yet; so reaches for an
older-version tool when it fits the job and he's less familiar with a
new potential replacement.
> There's nothing wrong with the (let [x ...] (assoc x :a :b)) pattern
> if you want to give each step a "name" (here x); but I'd like to point
> out in case you didn't notice that this could be written as
> (assoc ... :a :b).
Actually, he had
(let [x (foo bar)]
(assoc x :k (baz x)))
and x appears twice in the assoc. There may be a way to make that
cleaner, but it would not be as simple as getting rid of the "let".
> (let) already includes an implicit do; it's not clear why you're
> wrapping your let form with a do in do-file-wallpaper.
The do wraps only a single expression, the let; if he had a let around
a do it could be because he thought the let didn't produce an implicit
do, but the other way around?
My guess is the do is actually cruft left over from an earlier
revision that didn't get deleted when he refactored what was inside it
in a way that introduced the let. The odd line break in the middle of
a group of close parentheses, corresponding exactly to the boundary
between the end of the let s-expression and the remainder of the
enclosing do s-expression, is strong corroboration for this
hypothesis. It suggests the (defn ... (do ... )) structure
pre-existed, and then he decided to change the code inside the do; so
he found the close parenthesis of the do, hit enter, and now had the
nested stuff on a group of lines by itself without anything from
enclosing sexps. Then he worked on that stuff and at some point it
went from two unenclosed successive expressions to a single let form
surrounding same.
> Overall I'd say it's well-written; my nitpicks are pretty minor here.
Agree.
--
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
