[racket] Beginners request for code review.

I'm sorry for not responding until now - I read all the posts and am
very, very grateful for all the great comments and advice I got. I had
no time to do anything other than to read them though, I plan to take
my time tomorrow to implement most of them and to answer to all the
posts.

For now I'd like to note that I'm very positively surprised by the
responses I got and that every single "welcome to the club" made me
feel very proud and happy :)

Also, I wrote this in the email to Ian (which I accidentally sent to
him personally) and I want to say it once again: thank you, Matthias,
for writing a style guide for Racket. There are a (very) few points I
disagree with but overall it's a must read and I really don't know how
did I miss it and how I could have been missing it for a year. Is it
linked on the Racket page or in the documentation? It definitely
should be!

Again, thanks for wonderful feedback, I will get back to you tomorrow.

Best regards,
Piotr Klibert


2013/2/1 Marijn <hkBst at gentoo.org>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Poitr,
>
> On 31-01-13 02:20, Piotr Klibert wrote:
>> Hello all.
>>
>> TL;DR: I "want" a code review. Link to the repository at the end.
>
> [snip]
>
>> The code lives here:
>> http://bazaar.launchpad.net/~klibertp/+junk/bezier/files
>>
>> I will really appreciate any feedback from anyone. Thanks in
>> advance!
>
> In bezier-math.rkt you use:
>
> (define-syntax-rule (pow n) (* n n))
> (define-syntax-rule (inv n) (- 1.0 n))
>
> Your `pow' macro apparently is intended for squaring, and in fact
> there is a function predefined which does exactly that: `sqr'.  The
> function used for computing powers of numbers is called `expt'.
>
> Your `inv' macro seems to be calculating the negative of a number,
> except for the magic 1.0. Is there an off-by-one bug in the racket
> canvas that you're working around with this?  Ah, this is for use as
> the from-the-end parameter of the Bezier curve. Apparently you found
> it confusing too, because you sometimes preferred to spell it out.
>
> Your quadratic Bezier curve function:
>
> (define (bezier2 t a b c)
>   ;; I have no idea if this `let` helps or hurts performance.
>   ;; I have no idea why I wrote this like that either.
>   (let*
>       ([one-minus-t (inv t)]
>        [one-minus-t-squared (pow one-minus-t)]
>        [t-squared (pow t)])
>     (+ (* one-minus-t-squared a)
>        (* one-minus-t t b 2)
>        (* t-squared c))))
>
> I would write instead:
>
> (define (bezier2 a b c)
>   (lambda (t)
>     (let ((t^ (- 1 t)))
>       (+ (* a t^ t^)
>          (* 2 b t^ t)
>          (* c t t)))))
>
> That way you can use the same function for computations at different
> sample points.  I think you do that in bezier-struct.rkt:
>
> (define (point-at controls-x controls-y t)
>   (point (apply/bez t controls-x)
>          (apply/bez t controls-y)))
>
> In bezier-math.rkt again you have:
>
> ;; Wrappers accepting a list of points instead of points
> ;; directly. They delegated to `apply` at first but "unrolling"
> ;; arguments manually proved to be much more performant.
>
> (define-syntax-rule (apply/bez t pts)
>   (match pts
>     [(list a b c d) (bezier3 t a b c d)]))
>
> so presumably the repeated matching is hurting performance here. If
> you don't really need the control points, you don't even need a struct
> to hold them, but can use your bezier functions (closures) to
> ``remember'' the control points, like I showed above.
>
> So if I read bezier-struct.rkt correctly, it lets you precompute all
> necessary data as follows (conform make-curve):
>
> 1) construct a list of times between 0 and 1 (make-ts),
> 2) normalize control points from user input (make-controls),
> 3) for each time sample, split the controls by dimension and collect
> them into lists, match the lists against the arguments of bezier3,
> collect into a list of points (make-points),
> 4) for each time sample, split the controls by dimension and collect
> them into lists, match the lists against the arguments of deriv3 and
> call atan on the result, collect into a list of angles (make-angles).
> 5) construct a list of distances between adjacent sample points, by
> looping over list of points constructed in 3) (make-lengths).
> 6) convert all lists to vectors.
>
> Now if you care about the performance of that, then maybe you have
> noticed that 3) and 4) spend a lot of effort slicing and dicing lists
> instead of just computing bezier3 and deriv3. Maybe you also noticed
> that the preparatory slicing and dicing that 3) and 4) do is exactly
> the same! Even if you keep the design of bcurve unchanged you can
> eliminate a lot of unnecessary computation here to improve performance.
>
> If you want to do even better (faster, that is), then you will start
> to think about how to eliminate creating all these lists of
> precomputed data. Maybe the drawing routines could also be happy if
> you promise to do any computations they need done, when they need them
> (and throw them away again after, instead of putting them into a
> list). You probably don't want to do this by using real promises
> (streams, lazy lists).
>
> I hope that helps and good luck,
>
> Marijn
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.19 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlELlMYACgkQp/VmCx0OL2xmPACeIG8N3N3KoK10+Y4r2cpJ+7nk
> 3/UAoKKz4AtkEwhVg7fOAqKL1D7S8xWa
> =kYTV
> -----END PGP SIGNATURE-----