On 27/07/11 13:39, Jeremias Maerki wrote:
> On 27.07.2011 12:09:58 Vincent Hennebert wrote:
>> There is basic housekeeping that ought to be done IMO:
>>
>> On 26/07/11 19:28, jeremias wrote:
>>> Author: jeremias
>>> Date: Tue Jul 26 18:28:07 2011
>>> New Revision: 1151195
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1151195&view=rev
>>> Log:
>>> Fixed a bug in TTF subsetting where a composite glyph could get remapped 
>>> more than once resulting in garbled character.
>>>
>>> Modified:
>>>     
>>> xmlgraphics/fop/trunk/src/java/org/apache/fop/fonts/truetype/TTFSubSetFile.java
>>>     xmlgraphics/fop/trunk/status.xml
>>>
>> <snip/>
>>>      /**
>>> +     * We need to remember which composites were already remapped because 
>>> the value to be
>>> +     * remapped is being read from the TTF file and being replaced right 
>>> there. Doing this
>>> +     * twice would create a bad map the second time.
>>> +     */
>>> +    private Set<Long> remappedComposites = null;
>>
>> This should be put at the beginning of the file, along with all field
>> declarations. (Also, there’s no need to initialize it to null as it’s
>> already done by default.)
> 
> So we're back to nitpicking.

I actually find it very worrying that you consider this to be
nitpicking, when any decent book about software programming will
emphasize the importance of producing code that is as clear and readable
as possible. I think it’s urgent to improve the readability of our code
base if we want to attract more contributors. Could we commit to that?

It’s great that you java 1.5-ified parts of the code in that commit (has
it been tested though?), and it would be good if the other changes were
bringing the same improved clarity.


> I've done that intentionally to indicate
> that the variable is only just used by the following method.

By putting it at a non-expected place you’re making it difficult to find
the variable and understand in a quick glance what the class is made of.
This hampers the readability and maintainability of the code. Given that
it’s what we spend most of our time on, I find this worrying.

Your needing to put the variable near to the methods that use it is
a clear sign that this class is too big and needs to be split into
smaller entities.


> And the
> null is only there to emphasize that the variable is lazily assigned
> because the thing is often not even needed.

This is an interesting convention, although I believe it is cancelled out
by the fact that in a vast majority of cases, the initialization is
there just out of ignorance of Java’s default initialization. But that
doesn’t matter too much.


<snip/>
>>> +        if (remappedComposites.contains(offset)) {
>>> +            return;
>>
>> This return introduces another exit point that is hidden at the
>> beginning of the method. This is something that one wouldn’t expect and
>> makes the method hard to understand and maintain. This method should
>> most probably be split into smaller methods.
> 
> I'll swallow my comment to this and just do the split:
> http://svn.apache.org/viewvc?rev=1151447&view=rev

When I read this and the sarcastic message associated to the commit, I’m
concerned about the unwelcoming atmosphere that is being created on this
mailing list. Can we try and remain civil to each other?


>> But more importantly, there is no unit test that comes with this commit.
>> So there is no reason to believe that the problem is fixed and, most of
>> all, will not happen again in the future. Can you please add a unit test
>> for this?
> 
> No, I cannot. For licensing reasons. I can't upload the font that's
> causing this into the Apache SVN repository. I'd have to artificially
> construct a font that emulates this and I certainly won't try to do that.

We have the DejaVuLGCSerif font in our tests/resources/fonts directory.
Surely it must be possible to reproduce the issue with that font. Did
you have a look at it?


Thanks,
Vincent

Reply via email to