+1 on the documentation but the patch is too big for me to assess just
by reading it. It would be nice to separate the comments and reordering
of methods from more substantive refactorings and I'm all in favor of
making the APIs more uniform. We've been making headway in that area
recently. As an Eclipse user I'm insulated a bit from the textual
ordering of methods but I also like keeping statics, private and public
methods grouped in logical ways. Some of the current state is a
consequence of historical evolution and can be made more user friendly.
I will dig deeper into the patch this week.
I also suggest opening a JIRA (if you haven't already and I missed it)
so we can channel the discussion.
Jeff
On 8/12/10 3:30 PM, Ted Dunning wrote:
You are an ACE.
The documentation is something that we unqualifiedly need.
The code rearrangement sounds good in the abstract, but I would want to talk
about the details a bit. Opening a JIRA and posting a patch is a great way
to stimulate that discussion. Also, speaking as a good hypocrite
should (see MAHOUT-228), it would be good to have multiple small patches
rather than a large omnibus patch since we can dispose of the small ones in
less aggregate time than the large one.
Some of the things that you suggest aren't what I tend to do, but in style
issues, I value the experience of the naive reader over my own prejudices.
If you were confused when you read the code, I think we should improve the
readability. If it is something that you do "just because", then we should
probably both defer to the standard Mahout style rather than changing it.
On Thu, Aug 12, 2010 at 3:08 PM, Chris Wailes<[email protected]>wrote:
While doing so I decided to add some additional documentation based on what
I found out, re-ordered some of the functions so they could be found easier
(static and instance separated out), and re-formatted some of the code for
readability (added spaces between code blocks, broke long lines up into
multiple lines, and added 'this' to variables and function calls to make
context explicit).
