<div dir="ltr">(Piotr, sorry for the duplicate post, I meant to send it to the mailing list)<br><div><div><div class="gmail_quote"><div dir="ltr"><div><div><div><div><br></div>Not being myself a particularly good programmer, take the following with a grain of salt.<br>
</div><div>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, ...<br>
</div><div><br></div>Here are some unordered comments (along with some personal preferences), but really just for the sake of it:<br></div>- You can use rackunit for unit testing, and encapsulate them in a test sub-module (with `module+')<br>
- 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<br></div><div>- 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...)<br>
-<span> `which-control-clicked?</span>' <span></span>ends with a '?', which is in general reserved for predicates (could be renamed to `get-clicked-control' for example)<br></div><div>- You can use `send*' to avoid sequences of `(send dc ...)'<br>
</div><div>- `apply/deriv2' is probably useless (it doesn't even save a single character), but maybe that's for symmetry with the other operators?<br></div><div>- 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<br>
</div><div>- I also prefer not to put the true-arm of an if on the same line as the condition (like in `make-lengths')<br></div><div>- 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).<br>
</div><div>- 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.<br>
</div><div>- 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')<br>
<br></div><div>Performance aside, I probably would have written `make-lengths' more like this:<br><span style="font-family:courier new,monospace">(define (make-lengths points)<br> (for/fold ([sums '(0)])<br> ([current-point points] [last-point (rest points)])<br>
(cons (+ (first sums)<br> (points-distance last-point current-point))<br> sums)))</span><br><br></div>(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.)<br>
<br><div>To avoid code duplication, I may also have written `get-nearest-point' this way:<br><span style="font-family:courier new,monospace">(define (get-nearest-point curve dist)<br> (match (binarysearch (bcurve-lengths curve) dist)<br>
[(list b i) <br> (define (op-coord op-curve)<br> (vector-ref (op-curve curve) (if b i (sub1 i))))<br> (letter-pos (op-coord bcurve-points)<br> (op-coord bcurve-angles))]))</span></div><div>
<br></div><br><div></div><div><br></div><div>As you see, it's really for the sake of it...<br><br></div><div>Also: Nice demo! :)<span class="HOEnZb"><font color="#888888"><br><br></font></span></div><span class="HOEnZb"><font color="#888888"><div>
Laurent<br></div><div><br></div></font></span></div><div class="gmail_extra"><br><br>
<div class="gmail_quote"><div><div class="h5">On Thu, Jan 31, 2013 at 2:20 AM, Piotr Klibert <span dir="ltr"><<a href="mailto:piotr.klibert@10clouds.com" target="_blank">piotr.klibert@10clouds.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<div dir="ltr">Hello all.<div><br></div><div>TL;DR: I "want" a code review. Link to the repository at the end.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div><div><br></div>
<div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div><div>
<br></div><div>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).</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>And that's the problem.</div><div><br></div><div>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.</div>
<div><br></div><div>The code lives here: <a href="http://bazaar.launchpad.net/~klibertp/+junk/bezier/files" target="_blank">http://bazaar.launchpad.net/~klibertp/+junk/bezier/files</a> </div><div><br></div><div>I will really appreciate any feedback from anyone. Thanks in advance!</div>
<div><br></div><div><div><div dir="ltr">Best regards,<div>Piotr Klibert</div></div></div>
</div></div>
<br></div></div><div class="im">____________________<br>
Racket Users list:<br>
<a href="http://lists.racket-lang.org/users" target="_blank">http://lists.racket-lang.org/users</a><br>
<br></div></blockquote></div><br></div>
</div><br></div></div></div>