Vincent,
We apparently disagree on whether coding should be based on ideology or on
practical results. You appear to favor the former, I favor the latter. I
think we will have to leave it at that. I'm not going to alter my
programming style in order to adhere to your notion of ideal programming
practice. It would be one thing if there was an established consensus on
these matters based on objective reasoning, but there is not:
(1) the limit on file length appear to be arbitrary, and not a result of an
explicit reasoned process; i had reasons for coding the way I did (including
the use of nested classes), and I feel no need to alter or justify that
process; if someone can make an objectively reasoned argument on a case by
case basis, then I'm willing to consider refactoring at some point in the
future when other more important tasks are completed; e.g., I would be
willing to break out the nested class UnicodeBidiAlgorithm from
BidiUtil.java into a separate file (which would in address your comment
about separating bidi level determination from reordering);
(2) there is no standard for symbol length documented in FOP practice or
enforced by checkstyle; I decline to exchange my choice of symbols with
longer symbols simply because you prefer it that way; I have offered to add
comments to my uses, and that is the most I'm willing to do to address this
matter;
Note that in both of these cases, I am offering to take concrete steps to
address your comments, though not necessarily in the manner or to the full
extent you would prefer. This is called compromise, an important aspect of
working in a team.
G.
On Mon, Oct 24, 2011 at 6:54 PM, Vincent Hennebert <vhenneb...@gmail.com>wrote:
> On 22/10/11 01:22, Glenn Adams wrote:
> > inline
> >
> > On Sat, Oct 22, 2011 at 12:04 AM, Chris Bowditch <
> bowditch_ch...@hotmail.com
> >> wrote:
> >
> >>
> >> Since Thunderhead also needs this feature we are willing to invest some
> >> time into it too. Currently my team are telling me it would take 9
> person
> >> months to port this code into our branch of FOP, partly because of some
> >> merge conflicts, but also partly because we are not comfortable with
> some
> >> aspects of the code as it has already been pointed out. The estimate
> would
> >> include the time to refactor long files into small ones, deal with the
> >> variable names and restructuring the code.
> >>
> >
> > I would advise against this, since it would it is functionally
> unnecessary
> > and since it will make future merges more difficult. I will be making
> > additional changes as more features in this area are added.
> >
> > I don't see what refactoring long files into small ones buys you.
> However,
> > if you make a reasoned argument for factoring specific long files (i.e.,
> why
> > such factoring improves architecture, modularity, etc), rather than
> simply
> > say "all long files must be refactored", then I will seriously discuss
> doing
> > so post merge.
>
> When I read this I’m really puzzled because that should really be the
> other way around: what is the reason to keep those classes so big (and
> that must be a really good one)? Most likely the Single Responsibility
> Principle is being violated in those classes.
>
> BidiUtil is a good example of this: it computes Bidi levels on the FO
> tree, /and/ also re-orders areas on the area tree. Those two things
> should most probably be done in two separate classes. Especially since
> from the quick look I had they seem to be using two distinct sets or
> private methods.
>
>
> >> I appreciate your commitment to add comments to short identifiers
> >> declarations, so at least it will be easier for the team to translate
> the
> >> short variables to longer equivalents. Just so we are clear on what you
> >> propose, do you mean this:
> >>
> >> int gi = 0; // Glyph Index
> >>
> >
> > Yes. Note that I already do this in most cases, such as:
> >
> > private static void resolveExplicit ( int[] wca, int defaultLevel, int[]
> ea
> > ) {
> > int[] es = new int [ MAX_LEVELS ]; /* embeddings stack */
> > int ei = 0; /* embeddings stack index
> */
> > int ec = defaultLevel; /* current embedding
> level
> > */
> > for ( int i = 0, n = wca.length; i < n; i++ ) {
> > int bc = wca [ i ]; /* bidi class of current
> > char */
> > int el; /* embedding level to
> assign
> > to current char */
> > switch ( bc ) {
> > case LRE: // start left-to-right
> > embedding
> > case RLE: // start right-to-left
> > embedding
> > case LRO: // start
> left-to-right
> > override
> > case RLO: // start right-to-left
> > override
> > {
> > int en; /* new embedding level */
> > if ( ( bc == RLE ) || ( bc == RLO ) ) {
> > en = ( ( ec & ~OVERRIDE ) + 1 ) | 1;
> > } else {
> > en = ( ( ec & ~OVERRIDE ) + 2 ) & ~1;
> > }
> > if ( en < ( MAX_LEVELS + 1 ) ) {
> > es [ ei++ ] = ec;
> > if ( ( bc == LRO ) || ( bc == RLO ) ) {
> > ec = en | OVERRIDE;
> > } else {
> > ec = en & ~OVERRIDE;
> > }
> > } else {
> > // max levels exceeded, so don't change level or
> > override
> > }
> > el = ec;
> > break;
> > }
> > ...
> >
> > What I'm agreeing to do in the relative near future (after merge, before
> new
> > release) is to add such comments to those places where I have not already
> > done so, which are probably a minority of such cases.
>
> This is good to hear, although it only marginally helps. Again, what’s
> the rationale behind having 2 or 3 letter variables when every course
> about programming will emphasise the importance of having reasonably
> long, self-describing variable names? What amount of typing is there to
> save when any modern IDE will auto-complete variable names?
>
> Explaining the purpose of a variable in a comment creates two problems:
> first, it forces the developer to constantly scroll from where the
> variable is being used to where it is declared, in order to remember
> what its purpose is. By doing so, they lose the context in which the
> variable is used and have to read the code again. This makes it
> extremely painful and difficult to understand the code.
>
> But more importantly, there is no guarantee that the comment is
> accurate. It’s notorious that comments tend to be left behind when
> changes are made to the code. Which means that they quickly become
> misleading or even plain wrong. Therefore people tend to not trust
> comments, or even not read them at all.
>
> Choosing a self-explaining variable name solves both problems, while
> also forcing the programmer to re-think about the correctness of the
> code.
>
>
> Vincent
>
