On Thu, Oct 20, 2011 at 02:53:54PM +0100, Vincent Hennebert wrote:
> Here are the sizes of some new files:
> 1075 src/java/org/apache/fop/fonts/GlyphSequence.java
> 1089 src/java/org/apache/fop/fonts/GlyphProcessingState.java
> 1269
> src/codegen/unicode/java/org/apache/fop/text/bidi/GenerateBidiTestData.java
> 2034 src/java/org/apache/fop/layoutmgr/BidiUtil.java
> 3449 test/java/org/apache/fop/complexscripts/util/TTXFile.java
>
> This latter one contains more than 50 field
> declarations, and the Handler.startElement method alone is more than
> 1800 lines long.
>
> Also, the o.a.f.fonts.truetype.TTFFile class has now grown to
> 5502 lines. That’s 3 times its original size which was already too big.
> I regularly find myself looking at bits of this class, and I would be
> unable to do so on a 5500 line class.
I agree that these are undesirably long.
> I don’t think it needs to be explained why big classes are undesirable?
>
> Also, most files disable Checkstyle checks, the most important ones
> being line length and white space. Many files have too long lines which
> makes it a pain to read through, having to horizontally scroll all the
> time. We agreed on a certain coding style in the project and it would be
> good if new code could adhere to it.
I agree that this is not in compliance with the code style of the FOP
project.
> Speaking of variable names, here is a method picked from
> o.a.f.fonts.GlyphSequence:
> /**
> * Merge overlapping and abutting sub-intervals.
> */
> private static int[] mergeIntervals ( int[] ia ) {
> int ni = ia.length;
> int i, n, nm, is, ie;
> // count merged sub-intervals
> for ( i = 0, n = ni, nm = 0, is = ie = -1; i < n; i += 2 ) {
> int s = ia [ i + 0 ];
> int e = ia [ i + 1 ];
> if ( ( ie < 0 ) || ( s > ie ) ) {
> is = s;
> ie = e;
> nm++;
> } else if ( s >= is ) {
> if ( e > ie ) {
> ie = e;
> }
> }
> }
> int[] mi = new int [ nm * 2 ];
> // populate merged sub-intervals
> for ( i = 0, n = ni, nm = 0, is = ie = -1; i < n; i += 2 ) {
> int s = ia [ i + 0 ];
> int e = ia [ i + 1 ];
> int k = nm * 2;
> if ( ( ie < 0 ) || ( s > ie ) ) {
> is = s;
> ie = e;
> mi [ k + 0 ] = is;
> mi [ k + 1 ] = ie;
> nm++;
> } else if ( s >= is ) {
> if ( e > ie ) {
> ie = e;
> }
> mi [ k - 1 ] = ie;
> }
> }
> return mi;
> }
>
It could be refactored as follows:
/**
* Merge overlapping and abutting sub-intervals.
* @param inputArray The array to be merged
*/
private static int[] mergeIntervals ( int[] inputArray ) {
int numMerge = 0;
// count merged sub-intervals
for ( int i = 0, iCurrent = -1, iNext = -1; i < inputArray.length; i += 2 )
{
int current = inputArray [ i + 0 ];
int next = inputArray [ i + 1 ];
if ( ( iNext < 0 ) || ( current > iNext ) ) {
iCurrent = current;
iNext = next;
numMerge++;
} else if ( current >= iCurrent ) {
if ( next > iNext ) {
iNext = next;
}
}
}
int[] returnArray = new int [ numMerge * 2 ];
// populate merged sub-intervals
for ( int i = 0, numMerge2 = 0, iCurrent = -1, iNext = -1; i <
inputArray.length; i += 2 ) {
int current = inputArray [ i + 0 ];
int next = inputArray [ i + 1 ];
int numMerge2Square = numMerge2 * 2;
if ( ( iNext < 0 ) || ( current > iNext ) ) {
iCurrent = current;
iNext = next;
returnArray [ numMerge2Square + 0 ] = iCurrent;
returnArray [ numMerge2Square + 1 ] = iNext;
numMerge2++;
} else if ( current >= iCurrent ) {
if ( next > iNext ) {
iNext = next;
}
returnArray [ numMerge2Square - 1 ] = iNext;
}
}
return returnArray;
}
Many variables are now limited in scope to the loops. Only numMerge
and returnArray are visible outside the loop. The names make it
somewhat clearer what the code does (if I guessed the names right).
BTW Is there a requirement that the length of the input array is even?
If not, inputArray [ i + 1 ] will raise an exception if i == n-1.
So, yes, I agree with your objections against the actual format of the
code.
Simon
