Thanks for the review, Joe.
On 1/2/09 4:17 PM, "joenee...@gmail.com" <joenee...@gmail.com> wrote: > Reviewers: Carl.D.Sorensen, > > > http://codereview.appspot.com/11857/diff/1/2 > File input/regression/fret-diagrams.ly (right): > > http://codereview.appspot.com/11857/diff/1/2#newcode1 > Line 1: \version "2.12.0" > This regtest is getting quite large. Is there a logical way to split it > up (eg. fret-diagrams-landscape, fret-diagrams-string-count, etc)? The regtest can easily be split up. Is there a reason to do so? I would think that any time a change was made to the fret diagram code, the whole regtest would need to be run anyway. I'm not trying to be argumentative. I had the same impression, that the regtest was too long, but I needed all of those tests to make sure that everything worked properly. > > http://codereview.appspot.com/11857/diff/1/4 > File scm/fret-diagrams.scm (right): > > http://codereview.appspot.com/11857/diff/1/4#newcode1 > Line 1: ;;;; fret-diagrams.scm -- > I don't know enough about fret diagrams to really understand what's > going on, but I have one fairly general comment: the code is sprinkled > with > (cond ((eq? orientation 'landscape) > foo) > ((eq? orientation 'opposing-landscape) > bar) > (else > baz)) > where foo, bar and baz are almost the same except that they have > differences in signs and the X,Y are swapped around. It would be great > if this cond could be isolated to a few functions. For example: > > (define (real-coordinate string-coordinate fret-coordinate orientation) > (cond > ((eq? orientation 'landscape) > (cons fret-coordinate string-coordinate)) > ((eq? orientation 'opposing-landscape) > (cons (-fret-coordinate string-coordinate)) > (else > (cons string-coordinate fret-coordinate)))) Thanks for a great idea. I've hated the number of times I've checked orientation for what was basically the same thing. I hadn't thought about this kind of pattern. I'll look into it. > > and then make-straight-barre-stencil (for example) becomes > > (define (make-straight-barre-stencil > size half-thickness fret-coordinate > start-string-coordinate end-string-coordinate > orientation) > (let ((one-point (real-coordinate start-string-coordinate > fret-coordinate orientation)) > (other-point (real-coordinate end-string-coordinate > fret-coordinate orientation))) > (ly:make-line-stencil > half-thickness > (car one-point) (cdr one-point) > (car other-point) (cdr other-point))) > > If you're in a hurry to get the functionality in, don't let this hold > you up. It won't be in until release 2.12.2, so I have time to get it done right. > > http://codereview.appspot.com/11857/diff/1/4#newcode91 > Line 91: (cond > Here again you have cond leading to duplicate code. If you had > (define (combine-stencils a b c d) (ly:stencil-combine-at-edge a (car b) > (cdr b) c d)) > then you could move the cond inside: > > (combine-stencils string-stencil > (cond > ((eq? orientation 'landscape) (cons Y UP)) > ((eq? orientation 'opposing-landscape) (cons Y DOWN)) > (else (cons X RIGHT))) > (draw-strings (- string-count 1) fret-range th size orientation) > gap)) I'll go through them all again, and try to localize the orientation check. Thanks, Carl _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
