[ https://issues.apache.org/jira/browse/IGNITE-13531?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17208597#comment-17208597 ]
Alexey Zinoviev commented on IGNITE-13531: ------------------------------------------ [~mrkandreev] please, have a look, I've created a ticket for your clean-up edits, suggested in doc [https://docs.google.com/document/d/1_oBgmNfu6YnuSxEg9e1ImyGSV-fgmHq4Ut-hPq2bakQ/edit?usp=sharing] except the cloning (need to think how to do it better and maybe make it later by myself) > [ML] Code cleanup in Util classes > --------------------------------- > > Key: IGNITE-13531 > URL: https://issues.apache.org/jira/browse/IGNITE-13531 > Project: Ignite > Issue Type: Improvement > Reporter: Alexey Zinoviev > Priority: Major > Fix For: 2.10 > > > *Suggest improvement to Util classes* > > I suggest to add a final class modifier and to add a private constructor > to Util classes in ignite ml. This is Sonar rule RSPEC-1118 ( > [https://rules.sonarsource.com/java/tag/design/RSPEC-1118]). > > Motivation: > Utility classes, which are collections of static members, are not meant to > be instantiated. Even abstract utility classes, which can be extended, > should not have public constructors. Java adds an implicit public > constructor to every class which does not define at least one explicitly. > Hence, at least one non-public constructor should be defined. > > We can add this to: > * DistributedMetaStorageUtil.java > * ComputeUtils.java > * IgniteModelStorageUtil.java > * MapUtil.java > * MatrixUtil.java > * Utils.java > Class JdbcThinSSLUtil.java already has a private constructor. > > *Suggest add Serializable to Blas class* > I found that class Blas (org.apache.ignite.ml.math) is not Serializable but > fields f2jBlas and nativeBlas are transient. So I suggest adding > a Serializable to Blas class. > > *Add final modifier to static inner fields in utils class* > Motivation: > This static field public but not final, and could be changed by malicious > code or by accident from another package. The field could be made final to > avoid this vulnerability. > > For example replace: > public static IgniteDifferentiableDoubleToDoubleFunction SIGMOID = new > IgniteDifferentiableDoubleToDoubleFunction() { > } > With: > public static final IgniteDifferentiableDoubleToDoubleFunction SIGMOID = new > IgniteDifferentiableDoubleToDoubleFunction() { > } > > *Inefficient use of keySet iterator instead of entrySet* > This method accesses the value of a Map entry, using a key that was retrieved > from a keySet iterator. It is more efficient to use an iterator on the > entrySet of the map, to avoid the Map.get(key) lookup. > > Possible problem is expected order for set. > > For example: > for (Integer bucket : hist.keySet()) { > accum += hist.get(bucket); > res.put(bucket, accum); > } > > *Can be replaced with single Arrays.fill method call* > For example: > for (int i = 0; i < mins.length; i++) > mins[i] = Double.POSITIVE_INFINITY; > Can be replaced with: > Arrays.fill(mins, Double.POSITIVE_INFINITY); > Founded in: > * ImputerTrainer > * MaxAbsScalerTrainer > * MinMaxScalerTrainer -- This message was sent by Atlassian Jira (v8.3.4#803005)