You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mahout.apache.org by "Sean Owen (Created) (JIRA)" <ji...@apache.org> on 2011/12/03 22:14:39 UTC
[jira] [Created] (MAHOUT-913) Style changes / discussion
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
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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Dmitriy Lyubimov (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165449#comment-13165449 ]
Dmitriy Lyubimov commented on MAHOUT-913:
-----------------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Ted Dunning (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162238#comment-13162238 ]
Ted Dunning commented on MAHOUT-913:
------------------------------------
I was thinking of volatile and this case should be marked final rather than transient.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Dmitriy Lyubimov (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165478#comment-13165478 ]
Dmitriy Lyubimov commented on MAHOUT-913:
-----------------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162848#comment-13162848 ]
Hudson commented on MAHOUT-913:
-------------------------------
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
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162251#comment-13162251 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Jake Mannix (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162243#comment-13162243 ]
Jake Mannix commented on MAHOUT-913:
------------------------------------
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?
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sebastian Schelter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162310#comment-13162310 ]
Sebastian Schelter commented on MAHOUT-913:
-------------------------------------------
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.
> 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
[jira] [Issue Comment Edited] (MAHOUT-913) Style changes /
discussion
Posted by "Dmitriy Lyubimov (Issue Comment Edited) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165486#comment-13165486 ]
Dmitriy Lyubimov edited comment on MAHOUT-913 at 12/8/11 7:54 PM:
------------------------------------------------------------------
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).
was (Author: dlyubimov):
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 persisting 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).
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165466#comment-13165466 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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
[jira] [Issue Comment Edited] (MAHOUT-913) Style changes /
discussion
Posted by "Deneche A. Hakim (Issue Comment Edited) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167133#comment-13167133 ]
Deneche A. Hakim edited comment on MAHOUT-913 at 12/11/11 4:51 PM:
-------------------------------------------------------------------
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
was (Author: adeneche):
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 ?
> 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, Sean.xml
>
>
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Isabel Drost (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13163869#comment-13163869 ]
Isabel Drost commented on MAHOUT-913:
-------------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Ted Dunning (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162221#comment-13162221 ]
Ted Dunning commented on MAHOUT-913:
------------------------------------
+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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Dmitriy Lyubimov (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165611#comment-13165611 ]
Dmitriy Lyubimov commented on MAHOUT-913:
-----------------------------------------
bq. 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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Lance Norskog (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13163310#comment-13163310 ]
Lance Norskog commented on MAHOUT-913:
--------------------------------------
Eclipse has Checkstyle & PMD available as add-ons.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Deneche A. Hakim (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167102#comment-13167102 ]
Deneche A. Hakim commented on MAHOUT-913:
-----------------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Jake Mannix (Commented) (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Jake Mannix (Commented) (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Updated] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen updated MAHOUT-913:
-----------------------------
Attachment: MAHOUT-913.patch
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162232#comment-13162232 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Jeff Eastman (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162261#comment-13162261 ]
Jeff Eastman commented on MAHOUT-913:
-------------------------------------
+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?
> 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
[jira] [Updated] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen updated MAHOUT-913:
-----------------------------
Status: Patch Available (was: Open)
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Ted Dunning (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162234#comment-13162234 ]
Ted Dunning commented on MAHOUT-913:
------------------------------------
That usage of transient may be a substitute for an Atomic* object.
Where did you see the field?
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165496#comment-13165496 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Lance Norskog (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13163317#comment-13163317 ]
Lance Norskog commented on MAHOUT-913:
--------------------------------------
Does this break any active JIRA patches?
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162333#comment-13162333 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Deneche A. Hakim (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167133#comment-13167133 ]
Deneche A. Hakim commented on MAHOUT-913:
-----------------------------------------
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 ?
> 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, Sean.xml
>
>
> 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
[jira] [Updated] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen updated MAHOUT-913:
-----------------------------
Attachment: Sean.xml
My personal IJ inspections config preferences
> 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, Sean.xml
>
>
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Dmitriy Lyubimov (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165486#comment-13165486 ]
Dmitriy Lyubimov commented on MAHOUT-913:
-----------------------------------------
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 persisting 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).
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162236#comment-13162236 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
- 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?
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167136#comment-13167136 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
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.
> 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, Sean.xml
>
>
> 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
[jira] [Commented] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13162228#comment-13162228 ]
Sean Owen commented on MAHOUT-913:
----------------------------------
That's right. The only usage I saw this time was on a static field, which is never serialized (GSON or otherwise) anyway.
> 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
[jira] [Updated] (MAHOUT-913) Style changes / discussion
Posted by "Sean Owen (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MAHOUT-913?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sean Owen updated MAHOUT-913:
-----------------------------
Resolution: Fixed
Status: Resolved (was: Patch Available)
> 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