classpath
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: (patch) java.awt.geom - implementations


From: Sascha Brawer
Subject: Re: (patch) java.awt.geom - implementations
Date: Sat, 22 May 2004 00:12:58 +0200

Sven de Marothy <address@hidden> wrote on Fri, 21 May 2004 22:13:22 +0200:

>Here's a patch which implements some missing stuff in java.awt.geom.*
>
>The patch includes implementations of:
>contains() and intersects() for QuadCurve2D, CubicCurve2D as well
>as an implementation of GeneralPath, with documentation.

Thanks a lot, this is really great news! Please do not take it as a
criticism if I mention a few minor style-related things below -- if I may
say so, your code is already very nice in its current shape!

>It appears the current files were not formatted with Tom's GNU.xml
>jalopy file

Indeed. However, the changes to the comments and vars in

  public static void subdivide(double[], int, double[], int,
                               double[], int)

seem a bit weird. Maybe it shows a bug in GNU.xml, or in jalopy? The same
happened also in QuadCurve2D.solveQuadratic(). In any case, it would be
good to remove those replicated comments before check-in.

Sorry for being so picky here, but: It would be nice if all methods were
documented, including intersects(). This is actually my fault, not yours
-- I should have done this when I did the previous changes to that file.
But maybe you could fix it as part of your changes? Thanks!

Also, please note that our JavaDoc style uses the third person for the
initial sentence. For instance, the documentation of contains() should
thus start with "Determines", not with "Determine". Same with many other
methods. (Actually, there is plenty code in Classpath that does not
follow this particular convention...)

Do you have a reference to an explanation of the line-crossings
algorithm? If so, it might be good to add it to the respective method.
Maybe your nice write-up of the problem with figure-8 cubic paths could
go into the code docs? In the mailing list archive, it will be much
harder to find in case someone stumbles over the bug/feature.

getAxisIntersections(): Would it make sense putting 1E-10 in a constant?
(Also, the documentation is not according to the conventions).

GeneralPath (class comment): <p> before "The inside of the curve".
contains(double,double)/conyains(Point2D): The comment should start with
"/**".

contains(double, double, double, double): "return false", not "return
(false)".

>I did a little optimizing and benchmarking. It's a tad (~15%)
>slower for GeneralPath and QuadCurve2D, except for point-testing, 
>which is faster. CubicCurve2D seems signicantly faster though.

I'm just curious, do you know whether your replacement of points by
xpoints/ypoints has had much effect on performance? But please don't re-
write anything just for finding out...

>Ok. I guess I should write some Mauve-tests for this?

That would be really appreciated, because unit tests help tremendously
with ensuring code quality. But be aware of rounding issues in your
tests; bugs #7123 is still unresolved...

  http://savannah.gnu.org/bugs/index.php?func=detailitem&item_id=7123

In any case, many thanks for your nice patch, and I think I can say for
everyone that we are looking very much forward to your future contributions!

-- Sascha | address@hidden | www.brawer.ch






reply via email to

[Prev in Thread] Current Thread [Next in Thread]