[GitHub] [shiro] bmarwell commented on a diff in pull request #831: Refactor 2

["bmarwell (via GitHub)" <gi...@apache.org>]

bmarwell commented on code in PR #831:
URL: https://github.com/apache/shiro/pull/831#discussion_r1236824707


##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -209,17 +209,21 @@ public static Argon2Hash generate(
             int outputLengthBits
     ) {
         final int type;
+        Argon2Algorithm algorithm;
         switch (requireNonNull(algorithmName, "algorithmName")) {
             case "argon2i":
-                type = Argon2Parameters.ARGON2_i;
+                algorithm = new Argon2iAlgorithm();
+                type = algorithm.getType();
                 break;
             case "argon2d":
-                type = Argon2Parameters.ARGON2_d;
+                algorithm = new Argon2dAlgorithm();
+                type = algorithm.getType();
                 break;
             case "argon2":
                 // fall through
             case "argon2id":
-                type = Argon2Parameters.ARGON2_id;
+                algorithm = new Argon2idAlgorithm();
+                type = algorithm.getType();
                 break;

Review Comment:
   I fail to see the advantage of  this. You create a new class, but then just extract a parameter again. Yes it adds a little bit of polymorphism, but I do not see the advantage you mentioned here.
   Especially, `algorithm` is not used outside the switch, so why keep it outside? Why not just write `type = newArgon2dAlgorithm().type()`. But then, why encapsulate the type anyway?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org