[
https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162231#comment-13162231
]
Jake Mannix commented on MAHOUT-913:
------------------------------------
For missing license files, maybe we should set up the Rat maven plugin?
So apparently a lot of this is due to me, and as such, I apologize, I've been
out of the codebase for a while, so I've not been keeping in the same internal
habits and automated checkstyle setup etc. I would disagree that "Strong
engineering organizations wouldn't let basic style problems in the codebase" -
I've heard this repeatedly from multiple Xooglers, who seem to think that this
googly attitude is shared by everyone else in the industry. I've been in many
companies with strong engineering culture, and not all of them have the same
*level* of dedication to code hygiene.
That having been said, we've had much of this discussion before, and we have
all agreed that in this project we would stick to the various guidelines we've
imposed, and as such, any violations on my part are a mistake and I will try to
be more hygienic about it.
Some specific questions on this patch. It would probably be easier on a
reviewboard review (in fact, you could have commented on many of these things
in-line over at https://reviews.apache.org/r/2944/ before it was committed),
but I can see if I can ask about them here.
Some specifics that maybe I'm not remembering the logic on:
* What is wrong with 1d? Is it not the same as 1.0? It's 1 as a double
value, no?
* Why are so many private methods converted to static in this patch? If
they're private, nobody else is using them, so why make them static?
* And TODO/FIXME comments, why are you removing these? Seems like they leave
reminders in the code for what needs work to be done...
One thing I'd request: please revert too many changes in
InMemoryCollapsedVariationalBayes. There are fields and methods not being
used, but they *were*, and probably will be. That class is a work in progress,
and not necessary for "scalable" LDA, but is really nice for prototyping and
doing things at smaller scale, and deserves some more work. At very least,
let's just comment out the unused stuff marked with a TODO and link to a JIRA,
because otherwise I'll never find that stuff when I want to start working on it.
> Style changes / discussion
> --------------------------
>
> Key: MAHOUT-913
> URL: https://issues.apache.org/jira/browse/MAHOUT-913
> Project: Mahout
> Issue Type: Improvement
> Affects Versions: 0.5
> Reporter: Sean Owen
> Assignee: Sean Owen
> Priority: Minor
> Fix For: 0.6
>
> Attachments: MAHOUT-913.patch
>
>
> Guys I've still been seeing code committed that doesn't match standard Java
> style or a reasonable policy I can imagine. I wanted to talk about it since
> I've just been silently changing it and that is not ideal.
> This should be easy to get right, as automated tools exist to check and fix
> this. I recommend IntelliJ's free Community edition. Flip on even basic
> inspections. A hundred things will jump out (that are already jumping out at
> me). Most are automatically fixable.
> I think that standardized, readable code invites attention, work and care: it
> feels like something you want to improve, and don't want to hack up.
> I think it helps attract committers. Strong engineering organizations
> wouldn't let basic style problems in the codebase, just by using automated
> checks. Code reviews don't begin otherwise, and then reviews focus on real
> issues like design. We can make a basic effort to approach that level of
> quality. Otherwise, people who are used to a higher standard won't be
> inclined to participate in the project, and will just fork.
> I think it's a prerequisite to fixing real design issues, TODOs, correctness
> problems (cloning for instance), and refactorings. This code is not near that
> point, and won't get there at this rate.
> Personally it makes we want to only support anything I've written, and write
> any "next generation" recommender system in a new and separate venture. And
> I'm a friendly, and maybe not the only one! So would be great to keep some
> focus on quality and design.
> Here's a patch showing all the changes I've picked up and made with the IDE
> -- *just* basic style issues, and just since the last 2 weeks. The issues
> are, among others:
> ⁃ Empty javadoc
> ⁃ Redundant javadoc ("@param foo the foo")
> ⁃ Missing copyright headers
> ⁃ Copyright headers not at top of file (sometimes after imports!)
> ⁃ Very long lines (>> 120 chars)
> ⁃ "throws Exception" not on main() or test method
> ⁃ "transient" fields -- should never be used for us
> ⁃ Missing @Override
> ⁃ Using new Random()
> ⁃ Redundant boolean expressions like "foo == true"
> ⁃ Unused variables and parameters
> ⁃ Unused imports
> ⁃ Loops and conditionals without braces
> ⁃ Weird literals ("1d")
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
