mahout-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jake Mannix (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MAHOUT-913) Style changes / discussion
Date Sun, 04 Dec 2011 00:04:40 GMT

    [ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162241#comment-13162241
] 

Jake Mannix commented on MAHOUT-913:
------------------------------------

bq. I'd ask why a method that doesn't access instance state or methods not be marked as such
– static? (As they're private, it can't be because they may want to be overridden.) This
lets it be called without an instance, which it can be. In an extremely tight loop it saves
an invisible parameter passing too, but this is vanishingly small.

I tend to avoid static except when explicitly desired/required, and don't think I recall ever
seeing much use of "private static" in many codebases which I am familiar with.  But it doesn't
really matter to me, I just doubt that I'll remember to do that intentionally, as I don't
see the obvious benefit of the extra keyword.

1d I've done because it's clear that the only reason is that floating point division is desired.
 1.0 does the same job, as I'm probably the only person I know who always does 1d instead
of 1.0.

I'm down with the broken window theory, yeah, I guess there are just different kinds of things
which bother me, mostly around APIs and extensibility, not (most kinds of) purely stylistic
stuff.  I'm actually forcing myself to deal with the fact that the world has decided on the
horrible vertically misaligned braces choice every day, maybe that makes me blind to other
style issues.
                
> 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

       

Mime
View raw message