On Jan 18, 2011, at 10:59 AM, Tim Visher wrote:

> I'd love to have the code torn apart a little bit and get some
> suggestions for improvements.
> 
> It's located at https://gist.github.com/784744


It took me a few tries to figure out what to-wallpaper-name was doing. Maybe 
you should write a helper called split-filename or such, which you could use 
like (let [[base extension] (split-filename name)] ...). Also, why bother to 
negate the result of the name-has-resolution? test when you're going to do 
something in both cases anyway?

Why are you constructing your wallpaper structs in two steps (first creating 
without-destination-file and then assoc-ing the destination file to it)?

I'd chop the "to-" prefix off the front of most of your function names, as it 
feels redundant to me.

I'd also use map instead of for in wallpaper-resolutions, since it reads better 
to me with the list of resolutions coming at the end:

(map (partial apply struct resolution)
        [[1280 1024] [1920 1080] [2560 1600] [1920 1200] [1680 1050] [1440 900] 
[1280 800]])

Actually, you might consider just using 2-vectors for resolutions instead of 
structs/records. Besides being simpler, it would allow you to do things like 
(clojure.string/join "x" my-resolution).

-- 
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

Reply via email to