[racket] Fwd: Beginners request for code review.

(Piotr, sorry for the duplicate post, I meant to send it to the mailing
list)

Not being myself a particularly good programmer, take the following with a
grain of salt.
I agree with the others that this is some good piece of code. It's very
readable code: good variable names, similar s-expr are well aligned (not
sure everyone likes that though?), good module/procedure decomposition,
uses some nice features of Racket, ...

Here are some unordered comments (along with some personal preferences),
but really just for the sake of it:
- You can use rackunit for unit testing, and encapsulate them in a test
sub-module (with `module+')
- I try to avoid code duplication as much as possible (and Racket is
excellent for that), hence I would have abstracted over `make-points' and
`make-angles' in bezier-struct.rkt
- Not specific to racket, but I'd have liked some introducing text in the
main file to describe what this is about, what each file corresponds to,
and maybe some comments here and there for some specifics (and I'm guilty
of not doing that sufficiently myself...)
- `which-control-clicked?' ends with a '?', which is in general reserved
for predicates (could be renamed to `get-clicked-control' for example)
- You can use `send*' to avoid sequences of `(send dc ...)'
- `apply/deriv2' is probably useless (it doesn't even save a single
character), but maybe that's for symmetry with the other operators?
- It's a matter of taste, but I dislike when the condition does not start
on the same line as the if/when/unless, or when the opening paren of the
let/for/when and friends is not on the same line as the form itself
- I also prefer not to put the true-arm of an if on the same line as the
condition (like in `make-lengths')
- bezier-math.rkt/bezier2 looks too verbose to me. Probably `one-minus-t'
is the only binding I'd have made (not even really required, although the
`inv' name is a bit misleading).
- Likewise, some `let's here and there (e.g., `make-lengths') define
bindings that are used just once. Although giving informative variable
names make the code easier to read, it also makes it more verbose, which,
to me, has the opposite effect.
- when running bezier.rkt, there's a "(object:curved-text-editor% ...)"
being displayed. You can use `(void ...)' to avoid printing such things (or
bind it to some variable with `define')

Performance aside, I probably would have written `make-lengths' more like
this:
(define (make-lengths points)
  (for/fold ([sums '(0)])
            ([current-point points] [last-point (rest points)])
    (cons (+ (first sums)
             (points-distance last-point current-point))
          sums)))

(I used to put the `last-point' binding on a separate line, but nowadays
I'm not so convinced it's better in all circumstances. Maybe it's just a
phase.)

To avoid code duplication, I may also have written `get-nearest-point' this
way:
(define (get-nearest-point curve dist)
  (match (binarysearch (bcurve-lengths curve) dist)
    [(list b i)
     (define (op-coord op-curve)
       (vector-ref (op-curve curve) (if b i (sub1 i))))
     (letter-pos (op-coord bcurve-points)
                 (op-coord bcurve-angles))]))



As you see, it's really for the sake of it...

Also: Nice demo! :)

Laurent



On Thu, Jan 31, 2013 at 2:20 AM, Piotr Klibert
<piotr.klibert at 10clouds.com>wrote:

> Hello all.
>
> TL;DR: I "want" a code review. Link to the repository at the end.
>
> This is my first post here so I think it's appropriate to introduce
> myself. I'm a professional programmer and I have been programming for
> nearly twenty years now. At work I use Python and CoffeeScript; before that
> I used PHP, C++ and C, but I have no formal education in the field, which
> means that I missed an opportunity to learn about many interesting, but not
> immediately usable things.
>
> For the last few years I have been trying to compensate for this in my
> spare time. I have learnt, to various degrees, languages such as OCaml, F#,
> Smalltalk, Erlang, J and Prolog among other more or less interesting
> languages, dialects and environments.
>
> Still, until about a year ago I knew nothing about Lisps. I decided to
> take a look at one of them and by pure chance I chose Racket. I am in love
> with it ever since.
>
> For the last year I was scribbling Racket code from time to time and
> reading a lot about it: from docs to blog posts to source code. Everything
> I wrote was short and incomplete, I was essentially playing with newly
> discovered features without trying to build anything in particular.
>
> It took me so long, because Racket is vast and because I was concurrently
> playing with Erlang and Smalltalk. I still don't know much about Racket and
> I consider myself a beginner, but I decided that it's time to actually do
> something instead of just playing.
>
> Two weeks ago at work I was asked to prototype a JS app that would display
> on <canvas> and would allow a user to type some text and then bend it
> freely along the curve.
>
> I needed to figure a bit of math for this. HTML canvas API is rather low
> level one and essentially everything in the app would have to be computed
> by hand. I chose to play with math in Racket, using its GUI capabilities
> and I planned to switch to JS once the equations and basic algorithms were
> established. Which I did, the project started and the app is being
> developed right now (I'm not involved anymore).
>
> All this left me with about 200 lines of Racket code - the greatest amount
> I've written to date. I thought that this is the chance I was waiting for -
> all I had to do was to refactor the spaghetti into modules, add some event
> handlers and I would end up with a working Racket app.
>
> For a few days more I was cleaning up the code and optimizing it, trying
> to transform it into something I wouldn't be embarrassed of. I still didn't
> write a proper contracts nor docs, but I somehow succeeded - I have a
> little app that works and looks ok to my eyes.
>
> And that's the problem.
>
> I know very well that I'm a beginner and I know that there are vast parts
> of Racket that I don't know of. I would like to ask anyone who has a few
>  moments of free time to review what I wrote and tell me if it is a "good"
> - ok, passable - Racket code; what should I do to make it better, what
> mistakes I made and - maybe - what I managed to get right. I'm especially
> concerned with the style of the code and it's structure, but any comment is
> fine.
>
> The code lives here:
> http://bazaar.launchpad.net/~klibertp/+junk/bezier/files
>
> I will really appreciate any feedback from anyone. Thanks in advance!
>
> Best regards,
> Piotr Klibert
>
> ____________________
>   Racket Users list:
>   http://lists.racket-lang.org/users
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.racket-lang.org/users/archive/attachments/20130131/660ad457/attachment-0001.html>