[jira] [Commented] (IGNITE-13531) [ML] Code cleanup in Util classes

["Alexey Zinoviev (Jira)" <ji...@apache.org>]

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