[racket] Beginners request for code review.

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