[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Uses single algorithm for side-position spacing. (issue 6827072)
From: |
dak |
Subject: |
Uses single algorithm for side-position spacing. (issue 6827072) |
Date: |
Sun, 11 Nov 2012 13:50:14 +0000 |
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode85
lily/side-position-interface.cc:85: Position next to support, taking
into account my own dimensions and padding.
Incomprehensible comment. Is position verb or noun? What do your own
dimensions have to do with it?
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode88
lily/side-position-interface.cc:88: axis_aligned_side_helper (SCM smob,
Axis a, bool pure, int start, int end, SCM current_off_scm)
Why is the meaning of the arguments undocumented?
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode101
lily/side-position-interface.cc:101: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, x_aligned_side, 2, 1, "");
It is bad enough if you omit all comments from the code, but if the
declaration of a function _explicitly_ includes a DOC string, specifying
this as "" for a function with a vague name is not really helpful.
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode108
lily/side-position-interface.cc:108: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, y_aligned_side, 2, 1, "");
See above.
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode115
lily/side-position-interface.cc:115: MAKE_SCHEME_CALLBACK_WITH_OPTARGS
(Side_position_interface, pure_y_aligned_side, 4, 1, "");
See above.
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode144
lily/side-position-interface.cc:144: // long function - each stage is
clearly marked
Too bad that there is
a) no documentation what the long function does
b) no documentation what each step does.
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode275
lily/side-position-interface.cc:275: // necessary for the InstrumentName
grob
Well, if there is no _apparent_ logic to it, that would seem to warrant
explaining the logic, wouldn't it, instead of proudly announcing another
puzzle to the reader?
http://codereview.appspot.com/6827072/diff/1/lily/side-position-interface.cc#newcode309
lily/side-position-interface.cc:309: /* FIXME: 1000 should relate to
paper size. */
FIXME: the meaning of no variable at all is documented, so how should
the reader even know this is "paper size" related?
http://codereview.appspot.com/6827072/
- Uses single algorithm for side-position spacing. (issue 6827072),
dak <=
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/12
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/17