Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.6
    • Component/s: None
    • Labels:
      None

      Description

      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")

      1. Sean.xml
        59 kB
        Sean Owen
      2. MAHOUT-913.patch
        188 kB
        Sean Owen

        Activity

        Hide
        Ted Dunning added a comment -

        +1

        I apologize for not having time to help with this myself.

        The only question I have about your list is the transient. I think that the only use of this in memory has been for GSON which we eliminated.

        Show
        Ted Dunning added a comment - +1 I apologize for not having time to help with this myself. The only question I have about your list is the transient. I think that the only use of this in memory has been for GSON which we eliminated.
        Hide
        Sean Owen added a comment -

        That's right. The only usage I saw this time was on a static field, which is never serialized (GSON or otherwise) anyway.

        Show
        Sean Owen added a comment - That's right. The only usage I saw this time was on a static field, which is never serialized (GSON or otherwise) anyway.
        Hide
        Jake Mannix added a comment -

        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.

        Show
        Jake Mannix added a comment - 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.
        Hide
        Sean Owen added a comment -

        I don't know it's a level of dedication thing – it's about letting the machines handle what they can, so whatever level of effort is available can go to higher level things. I firmly believe in code hygiene, broken windows and all that, but that's just my experience. I don't enjoy trying to work on most of this project as a result.

        (Nothing is committed yet.)

        • It's obvious what 1.0 is. It is the same as 1d, but I think 1d is cryptic. Likewise 1 might be used as a double, and is equivalent to 1.0, so why not be clear and write 1.0 when it's used as 1.0?
        • 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 only removed the TODOs I thought I addressed, let me look at them again

        OK happy to revert those unused method/field changes.

        Show
        Sean Owen added a comment - I don't know it's a level of dedication thing – it's about letting the machines handle what they can, so whatever level of effort is available can go to higher level things. I firmly believe in code hygiene, broken windows and all that, but that's just my experience. I don't enjoy trying to work on most of this project as a result. (Nothing is committed yet.) It's obvious what 1.0 is. It is the same as 1d, but I think 1d is cryptic. Likewise 1 might be used as a double, and is equivalent to 1.0, so why not be clear and write 1.0 when it's used as 1.0? 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 only removed the TODOs I thought I addressed, let me look at them again OK happy to revert those unused method/field changes.
        Hide
        Ted Dunning added a comment -

        That usage of transient may be a substitute for an Atomic* object.

        Where did you see the field?

        Show
        Ted Dunning added a comment - That usage of transient may be a substitute for an Atomic* object. Where did you see the field?
        Hide
        Sean Owen added a comment -
        • private transient static Logger log = LoggerFactory.getLogger(SimpleTextEncodingVectorizer.class);

        It's OK. Why would transient have something to do with emulating Atomic* classes? Are you thinking of volatile?

        Show
        Sean Owen added a comment - private transient static Logger log = LoggerFactory.getLogger(SimpleTextEncodingVectorizer.class); It's OK. Why would transient have something to do with emulating Atomic* classes? Are you thinking of volatile?
        Hide
        Ted Dunning added a comment -

        I was thinking of volatile and this case should be marked final rather than transient.

        Show
        Ted Dunning added a comment - I was thinking of volatile and this case should be marked final rather than transient.
        Hide
        Jake Mannix added a comment -

        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.

        Show
        Jake Mannix added a comment - 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.
        Hide
        Jake Mannix added a comment -

        So regarding letting automated tools do their work - can you remind me / paint a picture of what you do when you're sanity checking style issues in the codebase? We've got a checkstyle file, so I guess I just need to reconfigure it to run automatically, and check the right reports on deprecation warnings and style violations.

        Couldn't we actually just put it part of the compile step? Such that mvn compile fails the build if there are violations? Then this code would never (or almost never) sneak into the codebase?

        Show
        Jake Mannix added a comment - So regarding letting automated tools do their work - can you remind me / paint a picture of what you do when you're sanity checking style issues in the codebase? We've got a checkstyle file, so I guess I just need to reconfigure it to run automatically, and check the right reports on deprecation warnings and style violations. Couldn't we actually just put it part of the compile step? Such that mvn compile fails the build if there are violations? Then this code would never (or almost never) sneak into the codebase?
        Hide
        Sean Owen added a comment -

        We do have mvn checkstyle available; I think it still works, is pretty well configured, and outputs and HTML report. FindBugs is hooked up too. I hesitate to fail a compile on warnings, but you could fail a commit. And really it would have to be new warnings. And some provision for exceptions. And I don't know how to do that with checkstyle, or if it's possible. Even I'm not worried about that yet.

        Honestly I just flip on the IntelliJ code inspections and tailor them a little bit. we have a config file around here; I could post mine. The nice thing is it can fix many of these things in one go once you're viewing the analysis report. Simple refactorings are of course available (like turning a public field into a private field + getters/setters, with corresponding code changes). Even before that it'll do nice things like grey out stuff that's unused. Additional plugins like Copyright can fix headers. Some search-and-replace with regexes does more. Some bits are manual. Eclipse does some of this but doesn't have the inspections.

        Show
        Sean Owen added a comment - We do have mvn checkstyle available; I think it still works, is pretty well configured, and outputs and HTML report. FindBugs is hooked up too. I hesitate to fail a compile on warnings, but you could fail a commit. And really it would have to be new warnings. And some provision for exceptions. And I don't know how to do that with checkstyle, or if it's possible. Even I'm not worried about that yet. Honestly I just flip on the IntelliJ code inspections and tailor them a little bit. we have a config file around here; I could post mine. The nice thing is it can fix many of these things in one go once you're viewing the analysis report. Simple refactorings are of course available (like turning a public field into a private field + getters/setters, with corresponding code changes). Even before that it'll do nice things like grey out stuff that's unused. Additional plugins like Copyright can fix headers. Some search-and-replace with regexes does more. Some bits are manual. Eclipse does some of this but doesn't have the inspections.
        Hide
        Jeff Eastman added a comment -

        +0.5 I see the value in having reasonably standard formatting and coding conventions but I'm not convinced standardizing on one tool makes sense in an open source project. I also have been silently annoyed by some of the large commits which have made kind of gratuitous changes to code I've written; making methods and fields static is one example I don't favor. I've mentioned this mildly before but have not pushed back much because the overall effect on the code base was positive and, heck, I did not have to do the work. I do favor well written JavaDocs and the Lucene standard formatting seems to be unreasonable enough, especially given our heritage. If we could automatically pretty-print every file on commit that would be something I would support. I'm just not wild about imposing a stricter set of conventions by other means. Is the situation really so grave?

        Show
        Jeff Eastman added a comment - +0.5 I see the value in having reasonably standard formatting and coding conventions but I'm not convinced standardizing on one tool makes sense in an open source project. I also have been silently annoyed by some of the large commits which have made kind of gratuitous changes to code I've written; making methods and fields static is one example I don't favor. I've mentioned this mildly before but have not pushed back much because the overall effect on the code base was positive and, heck, I did not have to do the work. I do favor well written JavaDocs and the Lucene standard formatting seems to be unreasonable enough, especially given our heritage. If we could automatically pretty-print every file on commit that would be something I would support. I'm just not wild about imposing a stricter set of conventions by other means. Is the situation really so grave?
        Hide
        Sebastian Schelter added a comment -

        I'm also in strong support of code conventions and consistent formatting. We have the issue that a lot of us only work in "their corner" of Mahout and different coding styles across different modules only amplify such a separation.

        Furthermore, as a publicly visible Apache project we should really aspire high quality, presentable code.

        Show
        Sebastian Schelter added a comment - I'm also in strong support of code conventions and consistent formatting. We have the issue that a lot of us only work in "their corner" of Mahout and different coding styles across different modules only amplify such a separation. Furthermore, as a publicly visible Apache project we should really aspire high quality, presentable code.
        Hide
        Sean Owen added a comment -

        I'm not suggesting standardizing on a tool, no. I do think these things are easier, since they are more automatic, with IntelliJ than Eclipse. It was a suggestion to lower the effort barrier, which could contribute to the issue. I actually don't like asking people to do more work, hopefully this is the opposite of that!

        We may just agree to disagree on private static methods; I think a non-instance method should be declared as such, and I don't think it's quite trivial. Consensus rules though, but I thought there was mostly agreement on this particular one.

        Formatting is most trivial and doesn't matter per se. I think it matters in that you probably have to be good at dealing with trivial issues before you can move up the stack, and there are most definitely less trivial issues I'd like to start talking about. If this is "level 0" (formatting) and "level 1" (redundant declarations), I'd like to start fixing "level 2" (wrong clone() or equals()/hashCode() pairs) and "level 3" (unencapsulated fields, polymorphic methods in constructors). Those are more real problems, and if you flip on inspections you'll see there are loads of them.

        Show
        Sean Owen added a comment - I'm not suggesting standardizing on a tool, no. I do think these things are easier, since they are more automatic, with IntelliJ than Eclipse. It was a suggestion to lower the effort barrier, which could contribute to the issue. I actually don't like asking people to do more work, hopefully this is the opposite of that! We may just agree to disagree on private static methods; I think a non-instance method should be declared as such, and I don't think it's quite trivial. Consensus rules though, but I thought there was mostly agreement on this particular one. Formatting is most trivial and doesn't matter per se. I think it matters in that you probably have to be good at dealing with trivial issues before you can move up the stack, and there are most definitely less trivial issues I'd like to start talking about. If this is "level 0" (formatting) and "level 1" (redundant declarations), I'd like to start fixing "level 2" (wrong clone() or equals()/hashCode() pairs) and "level 3" (unencapsulated fields, polymorphic methods in constructors). Those are more real problems, and if you flip on inspections you'll see there are loads of them.
        Hide
        Hudson added a comment -

        Integrated in Mahout-Quality #1225 (See https://builds.apache.org/job/Mahout-Quality/1225/)
        MAHOUT-913 many small style changes

        srowen : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1210428
        Files :

        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/TasteHadoopUtils.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/als/ALSUtils.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/item/RecommenderJob.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/preparation/PreparePreferenceMatrixJob.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/common/RunningAverageAndStdDev.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/model/file/FileDataModel.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/knn/KnnItemBasedRecommender.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/slopeone/MemoryDiffStorage.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/slopeone/file/FileDiffStorage.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/similarity/TanimotoCoefficientSimilarity.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/ConfusionMatrix.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/ResultAnalyzer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/Bagging.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/inmem/InMemMapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialBuilder.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/partial/Step1Mapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/ref/SequentialBuilder.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0DocInferenceMapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0Driver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0Mapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0PerplexityMapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/InMemoryCollapsedVariationalBayes0.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/ModelTrainer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/TopicModel.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/spectral/eigencuts/EigencutsDriver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/spectral/kmeans/SpectralKMeansDriver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/common/MemoryUtil.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/common/distance/MahalanobisDistanceMeasure.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/fpm/pfpgrowth/FPGrowthDriver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/MatrixUtils.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/DistributedLanczosSolver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/EigenVerificationJob.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/HdfsBackedLanczosState.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/solver/DistributedConjugateGradientSolver.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/SparseRowBlockWritable.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/YtYJob.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/qr/QRFirstStep.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/DictionaryVectorizer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/EncodingMapper.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/SimpleTextEncodingVectorizer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/Vectorizer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/VectorizerConfig.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/Weight.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/collocations/llr/CollocReducer.java
        • /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/collocations/llr/GramKeyPartitioner.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/cf/taste/hadoop/als/ParallelALSFactorizationJobTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/data/DataLoaderTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialBuilderTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialSequentialBuilder.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/canopy/TestCanopyCreation.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/lda/TestMapReduce.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/lda/cvb/TestCVBModelTrainer.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/common/distance/UserDefinedDistanceMeasure.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/graph/linkanalysis/PageRankJobTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/graph/linkanalysis/RandomWalkWithRestartJobTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/MathHelper.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/decomposer/TestDistributedLanczosSolver.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/solver/TestDistributedConjugateGradientSolverCLI.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/stochasticsvd/SSVDTestsHelper.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/math/stats/SamplerTest.java
        • /mahout/trunk/core/src/test/java/org/apache/mahout/vectorizer/EncodedVectorsFromSequenceFilesTest.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/FromEmailToDictionaryMapper.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MailToRecMapper.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MailToRecReducer.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MsgIdToDictionaryMapper.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/hadoop/example/als/netflix/NetflixDatasetConverter.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/NewsgroupHelper.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/RunAdaptiveLogistic.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/RunLogistic.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/SGDHelper.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/SGDInfo.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TestASFEmail.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TestNewsGroups.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainASFEmail.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainAdaptiveLogistic.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainLogistic.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainNewsGroups.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/canopy/Job.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/dirichlet/Job.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/meanshift/Job.java
        • /mahout/trunk/examples/src/main/java/org/apache/mahout/ga/watchmaker/cd/CDMutation.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/classifier/ConfusionMatrixDumper.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/SequenceFileDumper.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/clustering/ClusterDumper.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/clustering/ClusterWriter.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/AnalyzerTransformer.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/ChainTransformer.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/FPGFormatter.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/IdentityFormatter.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/IdentityTransformer.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexConverterDriver.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexFormatter.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexMapper.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexTransformer.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexUtils.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/URLDecodeTransformer.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/VectorDumper.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/arff/ARFFIterator.java
        • /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/arff/Driver.java
        • /mahout/trunk/integration/src/test/java/org/apache/mahout/utils/regex/RegexMapperTest.java
        • /mahout/trunk/integration/src/test/java/org/apache/mahout/utils/regex/RegexUtilsTest.java
        • /mahout/trunk/math/src/main/java/org/apache/mahout/math/MurmurHash3.java
        • /mahout/trunk/math/src/main/java/org/apache/mahout/math/als/AlternatingLeastSquaresSolver.java
        • /mahout/trunk/math/src/main/java/org/apache/mahout/math/als/ImplicitFeedbackAlternatingLeastSquaresSolver.java
        • /mahout/trunk/math/src/main/java/org/apache/mahout/math/decomposer/lanczos/LanczosState.java
        • /mahout/trunk/math/src/test/java/org/apache/mahout/math/MurmurHash3Test.java
        • /mahout/trunk/math/src/test/java/org/apache/mahout/math/decomposer/lanczos/TestLanczosSolver.java
        • /mahout/trunk/math/src/test/java/org/apache/mahout/math/solver/TestConjugateGradientSolver.java
        Show
        Hudson added a comment - Integrated in Mahout-Quality #1225 (See https://builds.apache.org/job/Mahout-Quality/1225/ ) MAHOUT-913 many small style changes srowen : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1210428 Files : /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/TasteHadoopUtils.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/als/ALSUtils.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/item/RecommenderJob.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/preparation/PreparePreferenceMatrixJob.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/common/RunningAverageAndStdDev.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/model/file/FileDataModel.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/knn/KnnItemBasedRecommender.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/slopeone/MemoryDiffStorage.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/recommender/slopeone/file/FileDiffStorage.java /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/impl/similarity/TanimotoCoefficientSimilarity.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/ConfusionMatrix.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/ResultAnalyzer.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/Bagging.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/inmem/InMemMapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialBuilder.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/mapreduce/partial/Step1Mapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/df/ref/SequentialBuilder.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0DocInferenceMapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CVB0Driver.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0Mapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/CachingCVB0PerplexityMapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/InMemoryCollapsedVariationalBayes0.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/ModelTrainer.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/cvb/TopicModel.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/spectral/eigencuts/EigencutsDriver.java /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/spectral/kmeans/SpectralKMeansDriver.java /mahout/trunk/core/src/main/java/org/apache/mahout/common/AbstractJob.java /mahout/trunk/core/src/main/java/org/apache/mahout/common/MemoryUtil.java /mahout/trunk/core/src/main/java/org/apache/mahout/common/distance/MahalanobisDistanceMeasure.java /mahout/trunk/core/src/main/java/org/apache/mahout/fpm/pfpgrowth/FPGrowthDriver.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/MatrixUtils.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/DistributedLanczosSolver.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/EigenVerificationJob.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/HdfsBackedLanczosState.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/solver/DistributedConjugateGradientSolver.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/SparseRowBlockWritable.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/YtYJob.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/qr/QRFirstStep.java /mahout/trunk/core/src/main/java/org/apache/mahout/math/stats/Sampler.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/DictionaryVectorizer.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/EncodingMapper.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/SimpleTextEncodingVectorizer.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/Vectorizer.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/VectorizerConfig.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/Weight.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/collocations/llr/CollocReducer.java /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/collocations/llr/GramKeyPartitioner.java /mahout/trunk/core/src/test/java/org/apache/mahout/cf/taste/hadoop/als/ParallelALSFactorizationJobTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/data/DataLoaderTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialBuilderTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/df/mapreduce/partial/PartialSequentialBuilder.java /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/canopy/TestCanopyCreation.java /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/lda/TestMapReduce.java /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/lda/cvb/TestCVBModelTrainer.java /mahout/trunk/core/src/test/java/org/apache/mahout/common/distance/UserDefinedDistanceMeasure.java /mahout/trunk/core/src/test/java/org/apache/mahout/graph/linkanalysis/PageRankJobTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/graph/linkanalysis/RandomWalkWithRestartJobTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/MathHelper.java /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/decomposer/TestDistributedLanczosSolver.java /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/solver/TestDistributedConjugateGradientSolverCLI.java /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/stochasticsvd/SSVDTestsHelper.java /mahout/trunk/core/src/test/java/org/apache/mahout/math/stats/SamplerTest.java /mahout/trunk/core/src/test/java/org/apache/mahout/vectorizer/EncodedVectorsFromSequenceFilesTest.java /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/FromEmailToDictionaryMapper.java /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MailToRecMapper.java /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MailToRecReducer.java /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/example/email/MsgIdToDictionaryMapper.java /mahout/trunk/examples/src/main/java/org/apache/mahout/cf/taste/hadoop/example/als/netflix/NetflixDatasetConverter.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/NewsgroupHelper.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/RunAdaptiveLogistic.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/RunLogistic.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/SGDHelper.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/SGDInfo.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TestASFEmail.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TestNewsGroups.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainASFEmail.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainAdaptiveLogistic.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainLogistic.java /mahout/trunk/examples/src/main/java/org/apache/mahout/classifier/sgd/TrainNewsGroups.java /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/canopy/Job.java /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/dirichlet/Job.java /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/syntheticcontrol/meanshift/Job.java /mahout/trunk/examples/src/main/java/org/apache/mahout/ga/watchmaker/cd/CDMutation.java /mahout/trunk/integration/src/main/java/org/apache/mahout/classifier/ConfusionMatrixDumper.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/SequenceFileDumper.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/clustering/ClusterDumper.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/clustering/ClusterWriter.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/AnalyzerTransformer.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/ChainTransformer.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/FPGFormatter.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/IdentityFormatter.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/IdentityTransformer.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexConverterDriver.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexFormatter.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexMapper.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexTransformer.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/RegexUtils.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/regex/URLDecodeTransformer.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/VectorDumper.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/arff/ARFFIterator.java /mahout/trunk/integration/src/main/java/org/apache/mahout/utils/vectors/arff/Driver.java /mahout/trunk/integration/src/test/java/org/apache/mahout/utils/regex/RegexMapperTest.java /mahout/trunk/integration/src/test/java/org/apache/mahout/utils/regex/RegexUtilsTest.java /mahout/trunk/math/src/main/java/org/apache/mahout/math/MurmurHash3.java /mahout/trunk/math/src/main/java/org/apache/mahout/math/als/AlternatingLeastSquaresSolver.java /mahout/trunk/math/src/main/java/org/apache/mahout/math/als/ImplicitFeedbackAlternatingLeastSquaresSolver.java /mahout/trunk/math/src/main/java/org/apache/mahout/math/decomposer/lanczos/LanczosState.java /mahout/trunk/math/src/test/java/org/apache/mahout/math/MurmurHash3Test.java /mahout/trunk/math/src/test/java/org/apache/mahout/math/decomposer/lanczos/TestLanczosSolver.java /mahout/trunk/math/src/test/java/org/apache/mahout/math/solver/TestConjugateGradientSolver.java
        Hide
        Lance Norskog added a comment -

        Eclipse has Checkstyle & PMD available as add-ons.

        Show
        Lance Norskog added a comment - Eclipse has Checkstyle & PMD available as add-ons.
        Hide
        Lance Norskog added a comment -

        Does this break any active JIRA patches?

        Show
        Lance Norskog added a comment - Does this break any active JIRA patches?
        Hide
        Isabel Drost-Fromm added a comment -

        http://code.google.com/p/dicode/source/browse/trunk/analysis/pom.xml <- what I did at another project to enforce correct headers (search for maven-enforcer-plugin) - it does not break the build but silently adds the headers during build time according to what you give it as header in an external file. Note that this will cause a lot of fixes on first build after adding the plugin if there are lots of headers missing.

        Show
        Isabel Drost-Fromm added a comment - http://code.google.com/p/dicode/source/browse/trunk/analysis/pom.xml <- what I did at another project to enforce correct headers (search for maven-enforcer-plugin) - it does not break the build but silently adds the headers during build time according to what you give it as header in an external file. Note that this will cause a lot of fixes on first build after adding the plugin if there are lots of headers missing.
        Hide
        Dmitriy Lyubimov added a comment -

        1) FWIW i always used 1d too.

        The reasoning is since there's no substitute for specifying 1l and 1f and it just so happens that 1.0 by default transforms into 1d but not 1f or 1l.

        Since you can't avoid using 1l and 1f where you need them (and you do need them esp. with in case of longs), there's no reason avoiding using 1d. In fact, using 1d is more explicit and conceptually coherent.

        single precision arithmetic case may be less evident these days (but still valid in case you want e.g. to save space with matrix stuff) but longs have a much stronger case. E.g. it should be evident that long a = (1<<31) and long a = (1l<<31) produce completely different results and there's no immediate substitute for 1l notation in the second case using default transformation rule such as 1.0.

        Am not arguing about need for uniform approach, just that 1d makes more sense to me.

        Show
        Dmitriy Lyubimov added a comment - 1) FWIW i always used 1d too. The reasoning is since there's no substitute for specifying 1l and 1f and it just so happens that 1.0 by default transforms into 1d but not 1f or 1l. Since you can't avoid using 1l and 1f where you need them (and you do need them esp. with in case of longs), there's no reason avoiding using 1d. In fact, using 1d is more explicit and conceptually coherent. single precision arithmetic case may be less evident these days (but still valid in case you want e.g. to save space with matrix stuff) but longs have a much stronger case. E.g. it should be evident that long a = (1<<31) and long a = (1l<<31) produce completely different results and there's no immediate substitute for 1l notation in the second case using default transformation rule such as 1.0. Am not arguing about need for uniform approach, just that 1d makes more sense to me.
        Hide
        Sean Owen added a comment -

        1d and 1f look like hex literals to me – at the least, I'd argue for 1.0d and 1.0f. They're obviously the floating point values that they are that way.

        Of course, if you mean to use a floating-point or long literal you most certainly must use these qualifiers. No argument there, at all. (1l ought to be 1L to avoid confusion with eleven!) 'd' is never needed to explicitly qualify a double literal.

        "1" is an int and there is no qualifier for int (right?). "1L" is long and needs a qualifier. "1" is the default not because of its precision but just because it's the more commonly used type. By symmetry I'd expect conventional use to be "1.0" and "1.0f"; I actually don't know why there's a 'd'.

        I'd argue that convention is strongly against 1.0d; I've not seen it used anywhere (other than Mahout), it's not in Sun's style guide, it's flagged as "confusing" by IntelliJ by default, etc. That's not to say it's wrong, just a statement about what is probably unsurprising for most developers.

        Show
        Sean Owen added a comment - 1d and 1f look like hex literals to me – at the least, I'd argue for 1.0d and 1.0f. They're obviously the floating point values that they are that way. Of course, if you mean to use a floating-point or long literal you most certainly must use these qualifiers. No argument there, at all. (1l ought to be 1L to avoid confusion with eleven!) 'd' is never needed to explicitly qualify a double literal. "1" is an int and there is no qualifier for int (right?). "1L" is long and needs a qualifier. "1" is the default not because of its precision but just because it's the more commonly used type. By symmetry I'd expect conventional use to be "1.0" and "1.0f"; I actually don't know why there's a 'd'. I'd argue that convention is strongly against 1.0d; I've not seen it used anywhere (other than Mahout), it's not in Sun's style guide, it's flagged as "confusing" by IntelliJ by default, etc. That's not to say it's wrong, just a statement about what is probably unsurprising for most developers.
        Hide
        Dmitriy Lyubimov added a comment -

        Agree on capitals. Yes capitals are better.

        so what's your bottom line for the entire list?

        1.0F for floats
        1.0 for doubles
        1L for longs
        1 for ints

        (or perhaps

        1F for floats
        1.0 for doubles
        1L for longs
        1 for ints
        ) ?

        I don't care much, I just have to know for Mahout's case.

        Show
        Dmitriy Lyubimov added a comment - Agree on capitals. Yes capitals are better. so what's your bottom line for the entire list? 1.0F for floats 1.0 for doubles 1L for longs 1 for ints (or perhaps 1F for floats 1.0 for doubles 1L for longs 1 for ints ) ? I don't care much, I just have to know for Mahout's case.
        Hide
        Dmitriy Lyubimov added a comment - - edited

        also RE: transients:

        Yes transients don't apply. i may have used them in Writables as markers (similar to "marker interface") concept to denote fields that actually do not get serialized.

        Yes Writable serialization is not java serialization and transient keyword use is wrong. But perhaps some standard annotation would help as a marker, i am not sure. Comments are usually not as helpful because they are in human language istead of "keyword" language and don't have a standard look the eye gets used to and grabs on. So i don't know. But not to mark nonserialized fields in Writables is kind of dangerous.

        One may also argue that persisted objects (such as Writables) must not have a non-persisted state... But pragmatic situations suggest that would be too much of an arm twisting in certain cases (e.g. caching a frequently used knowledge derived from minimally required persisted state in an instantiated object).

        Show
        Dmitriy Lyubimov added a comment - - edited also RE: transients: Yes transients don't apply. i may have used them in Writables as markers (similar to "marker interface") concept to denote fields that actually do not get serialized. Yes Writable serialization is not java serialization and transient keyword use is wrong. But perhaps some standard annotation would help as a marker, i am not sure. Comments are usually not as helpful because they are in human language istead of "keyword" language and don't have a standard look the eye gets used to and grabs on. So i don't know. But not to mark nonserialized fields in Writables is kind of dangerous. One may also argue that persisted objects (such as Writables) must not have a non-persisted state... But pragmatic situations suggest that would be too much of an arm twisting in certain cases (e.g. caching a frequently used knowledge derived from minimally required persisted state in an instantiated object).
        Hide
        Sean Owen added a comment -

        I write: 1 (int), 1.0 (double), 1L (long), 1.0f (float). (Even I don't have a view on 'f' vs 'F'). Just because it seems simplest.

        The transient I saw was on a static field, which wouldn't apply even for Serializable. Since transient only has meaning for Serializable it seems less than ideal as a marker. It is a keyword since it's supposed to have a particular effect, and if applied where it can't have that effect, looks like an error.

        For us I don't know of any fields in a Writable that is not serialized, so it may be a moot point at this stage to figure out how to label such a thing.

        Show
        Sean Owen added a comment - I write: 1 (int), 1.0 (double), 1L (long), 1.0f (float). (Even I don't have a view on 'f' vs 'F'). Just because it seems simplest. The transient I saw was on a static field, which wouldn't apply even for Serializable. Since transient only has meaning for Serializable it seems less than ideal as a marker. It is a keyword since it's supposed to have a particular effect, and if applied where it can't have that effect, looks like an error. For us I don't know of any fields in a Writable that is not serialized, so it may be a moot point at this stage to figure out how to label such a thing.
        Hide
        Dmitriy Lyubimov added a comment -

        The transient I saw was on a static field

        not me then I am not sure if i use non-serialized fields in Writables in Mahout at all at this point. I don't think i created many Writables in Mahout.

        so yes it looks like a moot point.

        ok 1L 1.0f. Got it.

        Show
        Dmitriy Lyubimov added a comment - The transient I saw was on a static field not me then I am not sure if i use non-serialized fields in Writables in Mahout at all at this point. I don't think i created many Writables in Mahout. so yes it looks like a moot point. ok 1L 1.0f. Got it.
        Hide
        Deneche A. Hakim added a comment -

        Sean, could you post your IntelliJ config file for the code inspection ? I am committing a patch soon, so it's better to check the style now.

        Show
        Deneche A. Hakim added a comment - Sean, could you post your IntelliJ config file for the code inspection ? I am committing a patch soon, so it's better to check the style now.
        Hide
        Sean Owen added a comment -

        My personal IJ inspections config preferences

        Show
        Sean Owen added a comment - My personal IJ inspections config preferences
        Hide
        Deneche A. Hakim added a comment - - edited

        about javadoc, if a method has an attribute "a", but I don't know how to describe it, is it best to have @param a with a missing description or have the @param missing all together ?

        edit: Thanks Sean for the style file

        Show
        Deneche A. Hakim added a comment - - edited about javadoc, if a method has an attribute "a", but I don't know how to describe it, is it best to have @param a with a missing description or have the @param missing all together ? edit: Thanks Sean for the style file
        Hide
        Sean Owen added a comment -

        You mean it has a parameter "a"? I would not write an empty "@param a"; Javadoc already shows the name and type of all parameters. I suppose if you just can't describe it, don't write anything.

        Show
        Sean Owen added a comment - You mean it has a parameter "a"? I would not write an empty "@param a"; Javadoc already shows the name and type of all parameters. I suppose if you just can't describe it, don't write anything.

          People

          • Assignee:
            Sean Owen
            Reporter:
            Sean Owen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development