small code review..
Martin Hauner
I'm doing my first steps in Clojure and I would like to get some
feedback on my implementation of the Bowling Game Kata. The code is
here http://softnoise.wordpress.com/2010/02/07/the-bowling-kata-in-clojure.
I can post the code here if that's preferred.
I'm mostly interested if there is anything in my code one would/should
never do that way.
Thanks
Martin
Brenton
It's very simple, I like it.
There two things that stand out to me, both having to do with
readabiliby.
Instead of using (def score) you should use (declare score). declare
exists for the purpose of making forward declarations. That one is
black and white. You have to do it.
The other I am not so sure about. You create a function named sum
which calls (reduce + rolls). I think that an experienced Clojure
programmer would rather just see
(reduce + (take 2 rolls)) rather than (sum (take 2 rolls)). There are
two reasons you may want to create the sum function. One bad and one
not so bad. The first is that you think reduce + is confusing and so
you are creating a name that makes more sense to you. The second is
that you are going to be using it a lot and you want to save 5
keystrokes. I have issue with the first reason. When an experienced
Clojure dev sees (reduce + (take 2 rolls)) there is only one foreign
idea to think about, namely rolls. When they see (sum (take 2 rolls))
the complexity has increased because they now they have two foreign
ideas to think about. Another way to think about it is that when I see
(reduce + (take 2 rolls)) I know exactly what it does. When I see (sum
(take 2 rolls)), I don't know what it does. sum could be some crazy,
messed up side effecting function that could cause all kinds of
problems. In your code it is easy to check that sum is correct but
what if it was implemented in another file?
I only point this out because I find myself doing this all the time.
Brenton
On Feb 11, 9:19 am, Martin Hauner <martin.hau...@gmx.net> wrote:
> Hi,
>
> I'm doing my first steps in Clojure and I would like to get some
> feedback on my implementation of the Bowling Game Kata. The code is
> herehttp://softnoise.wordpress.com/2010/02/07/the-bowling-kata-in-clojure.
Timothy Pratley
> I'm mostly interested if there is anything in my code one would/should
> never do that way.
Looks excellent, you've aced it!
I'd like to discuss switching...
You wrote:
(cond
(empty? rolls) 0
(strike? rolls) (+ (score-strike rolls) (score-after-strike rolls))
(spare? rolls) (+ (score-spare rolls) (score-after-frame rolls))
(more? rolls) (+ (score-frame rolls) (score-after-frame rolls)))
Which is correct and very readable.
If I wanted a DRYer expression:
(condp #(%1 %2) rolls
empty? 0
strike? (+ (score-strike rolls) (score-after-strike rolls))
spare? (+ (score-spare rolls) (score-after-frame rolls))
more? (+ (score-frame rolls) (score-after-frame rolls)))
But the #(%1 %2) is ugly and not immediately obvious how it works.
Would it be any nicer if we defined a function-name?
or made a switchp which hides that part?
Or use a trick:
(condp apply [rolls]
....
....)
I've found myself faced with this particular DRY vs clear trade-off
and never been sure what to do!
Regards,
Tim.
ataggart
On Feb 11, 4:34 pm, Timothy Pratley <timothyprat...@gmail.com> wrote:
> On 12 February 2010 03:19, Martin Hauner <martin.hau...@gmx.net> wrote:
>
> > I'm mostly interested if there is anything in my code one would/should
> > never do that way.
>
> Looks excellent, you've aced it!
>
> I'd like to discuss switching...You wrote:
>
> (cond
> (empty? rolls) 0
> (strike? rolls) (+ (score-strike rolls) (score-after-strike rolls))
> (spare? rolls) (+ (score-spare rolls) (score-after-frame rolls))
> (more? rolls) (+ (score-frame rolls) (score-after-frame rolls)))
>
> Which is correct and very readable.
>
> If I wanted a DRYer expression:
> (condp #(%1 %2) rolls
> empty? 0
> strike? (+ (score-strike rolls) (score-after-strike rolls))
> spare? (+ (score-spare rolls) (score-after-frame rolls))
> more? (+ (score-frame rolls) (score-after-frame rolls)))
>
> But the #(%1 %2) is ugly and not immediately obvious how it works.
> Would it be any nicer if we defined a function-name?
> or made a switchp which hides that part?
> Or use a trick:
> (condp apply [rolls]
> ....
> ....)
>
> I've found myself faced with this particular DRY vs clear trade-off
> and never been sure what to do!
>
> Regards,
> Tim.
And the minute you change cond to be more terse someone will decide
that one of the conditions needs to factor in some other param, thus
breaking the pattern. ;)
Martin Hauner
On 11 Feb., 19:59, Brenton <bashw...@gmail.com> wrote:
> Martin,
>
> It's very simple, I like it.
>
> There two things that stand out to me, both having to do with
> readabiliby.
>
> Instead of using (def score) you should use (declare score). declare
> exists for the purpose of making forward declarations. That one is
> black and white. You have to do it.
I can't remember if i looked that up somewhere or just tried it and it
worked. I guess
I was confused because the doc of declare speaks of vars not
functions. It wasn't
clear to me then that defn function creates a var too.
> The other I am not so sure about. You create a function named sum
> which calls (reduce + rolls). I think that an experienced Clojure
> programmer would rather just see
> (reduce + (take 2 rolls)) rather than (sum (take 2 rolls)). There are
> two reasons you may want to create the sum function. One bad and one
> not so bad. The first is that you think reduce + is confusing and so
> you are creating a name that makes more sense to you. The second is
> that you are going to be using it a lot and you want to save 5
> keystrokes. I have issue with the first reason.
I think i created the function to reduce duplication, but I see that
(reduce) is such
a common construct that my sum function may not be an improvement.
> I only point this out because I find myself doing this all the time.
>
> Brenton
Thanks for your feedback,
Martin
Martin Hauner
> On 12 February 2010 03:19, Martin Hauner <martin.hau...@gmx.net> wrote:
>
> > I'm mostly interested if there is anything in my code one would/should
> > never do that way.
>
> Looks excellent, you've aced it!
Thanks :-)
> I'd like to discuss switching...You wrote:
>
> (cond
> (empty? rolls) 0
> (strike? rolls) (+ (score-strike rolls) (score-after-strike rolls))
> (spare? rolls) (+ (score-spare rolls) (score-after-frame rolls))
> (more? rolls) (+ (score-frame rolls) (score-after-frame rolls)))
>
> Which is correct and very readable.
>
> If I wanted a DRYer expression:
> (condp #(%1 %2) rolls
> empty? 0
> strike? (+ (score-strike rolls) (score-after-strike rolls))
> spare? (+ (score-spare rolls) (score-after-frame rolls))
> more? (+ (score-frame rolls) (score-after-frame rolls)))
>
> But the #(%1 %2) is ugly and not immediately obvious how it works.
> Would it be any nicer if we defined a function-name?
> or made a switchp which hides that part?
I have to understand condp step by step.
We have (condp pred expr) with (test-expr result-expr) and condp does
call
(pred test-expr expr). With pred = #(%1 %2), expr = rolls, test-expr =
empty? etc..
So it finally calls (empty? rolls).
I see, but I have to admit that I prefer the simple cond. At least for
now ;-)
> Or use a trick:
> (condp apply [rolls]
> ....
> ....)
Indeed, works too.
Trying to understand that one now..
pred = apply, exp = [rolls], condp calls (apply empty? [rolls]) and
apply calls
(empty? rolls).
I like that better than the anonymous function.
Someone suggested to use a (binding) to avoid the repeating rolls
parameter. He wasn't sure if
it would be ok (learning too).
Since I have to change the code to calculate (score) in parallel
anyway it may be ok to make
rolls thread locale!?
> I've found myself faced with this particular DRY vs clear trade-off
> and never been sure what to do!
>
> Regards,
> Tim.
Thanks
Martin
Meikel Brandmeyer
On Sun, Feb 14, 2010 at 08:32:42AM -0800, Martin Hauner wrote:
> > (cond
> > (empty? rolls) 0
> > (strike? rolls) (+ (score-strike rolls) (score-after-strike rolls))
> > (spare? rolls) (+ (score-spare rolls) (score-after-frame rolls))
> > (more? rolls) (+ (score-frame rolls) (score-after-frame rolls)))
>
> Someone suggested to use a (binding) to avoid the repeating rolls
> parameter. He wasn't sure if
> it would be ok (learning too).
> Since I have to change the code to calculate (score) in parallel
> anyway it may be ok to make
> rolls thread locale!?
I think the cond – as you did – is the best way to go. condp is supposed
to be used with a single predicate testing against different values.
The suggested condp forms are pushing this a little bit too much, IMHO.
How binding could used in this case is rather unclear to me…
Just me two 0.02€.
Sincerely
Meikel