2
\$\begingroup\$

There is a lot of code duplication because it generates hashes using multiple cryptographic hash algorithms.

How can I improve this code?

package main;
import java.util.Scanner;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

public class String_Hash_Generator {
    public static void main(String args[]) throws NoSuchAlgorithmException {
        Scanner inputScanner = new Scanner(System.in);
        System.out.print("String: ");
        String input = inputScanner.next();

        /* MD2 */
        MessageDigest objMD2 = MessageDigest.getInstance("MD2");
        byte[] bytMD2 = objMD2.digest(input.getBytes());
        BigInteger intNumMD2 = new BigInteger(1, bytMD2);
        String hcMD2 = intNumMD2.toString(16);
        while (hcMD2.length() < 32) {
            hcMD2 = "0" + hcMD2;
        }

        /* MD5 */
        MessageDigest objMD5 = MessageDigest.getInstance("MD5");
        byte[] bytMD5 = objMD5.digest(input.getBytes());
        BigInteger intNumMD5 = new BigInteger(1, bytMD5);
        String hcMD5 = intNumMD5.toString(16);
        while (hcMD5.length() < 32) {
            hcMD5 = "0" + hcMD5;
        }

        /* SHA-1 */
        MessageDigest objSHA1 = MessageDigest.getInstance("SHA-1");
        byte[] bytSHA1 = objSHA1.digest(input.getBytes());
        BigInteger intNumSHA1 = new BigInteger(1, bytSHA1);
        String hcSHA1 = intNumSHA1.toString(16);
        while (hcSHA1.length() < 40) {
            hcSHA1 = "0" + hcSHA1;
        }


        /* SHA-256 */
        MessageDigest objSHA256 = MessageDigest.getInstance("SHA-256");
        byte[] bytSHA256 = objSHA256.digest(input.getBytes());
        BigInteger intNumSHA256 = new BigInteger(1, bytSHA256);
        String hcSHA256 = intNumSHA256.toString(16);
        while (hcSHA256.length() < 64) {
            hcSHA256 = "0" + hcSHA256;
        }

        /* SHA-384 */

        MessageDigest objSHA384 = MessageDigest.getInstance("SHA-384");
        byte[] bytSHA384 = objSHA384.digest(input.getBytes());
        BigInteger intNumSHA384 = new BigInteger(1, bytSHA384);
        String hcSHA384 = intNumSHA384.toString(16);
        while (hcSHA384.length() < 96) {
            hcSHA384 = "0" + hcSHA384;
        }

        /* SHA-512 */
        MessageDigest objSHA512 = MessageDigest.getInstance("SHA-512");
        byte[] bytSHA512 = objSHA512.digest(input.getBytes());
        BigInteger intNumSHA512 = new BigInteger(1, bytSHA512);
        String hcSHA512 = intNumSHA512.toString(16);
        while (hcSHA512.length() < 128) {
            hcSHA512 = "0" + hcSHA512;
        }

        System.out.println("\nMD2: " + hcMD2
                        + "\nMD5: " + hcMD5
                        + "\nSHA-1: " + hcSHA1
                        + "\nSHA-256: " + hcSHA256
                        + "\nSHA-384: " + hcSHA384
                        + "\nSHA-512: " + hcSHA512);
    }
}
\$\endgroup\$
  • \$\begingroup\$ I suggest you use String.format() for the formatting (gets rid of the while loops). Also: Don't use BigInteger, it's painfully slow. Look at this answer: stackoverflow.com/a/473309/4867727 \$\endgroup\$ – GiantTree Aug 5 '16 at 23:56
  • \$\begingroup\$ @GiantTree I have never really used String.format() before now. Could you provide an example of modified code so I could understand exactly what you're talking about? \$\endgroup\$ – Anonymous Aug 6 '16 at 0:15
1
\$\begingroup\$

Instead of using while loops (even though they might increase performance with StringBuffer), use String.format():

So instead of this:

String hcMD2 = intNumMD2.toString(16);
while (hcMD2.length() < 32) {
        hcMD2 = "0" + hcMD2;
}

You can simply write this:

hcMD2 = String.format("%032x", intNumMD2);

The formatting string %032x means:
0: pad with 0
32: to a length of 32 characters
x: as a hexadecimal integer

\$\endgroup\$
  • \$\begingroup\$ Using this, could I also delete the line String hcMD2 = intNumMD2.toString(16);? \$\endgroup\$ – Anonymous Aug 6 '16 at 0:31
  • \$\begingroup\$ Yes, I'll add that to the answer. \$\endgroup\$ – GiantTree Aug 6 '16 at 0:31
  • \$\begingroup\$ This improved the run time by about a second (big improvement because I'll be implementing this to run on many machines constantly). It also reduced the code by 18 lines. Thanks! \$\endgroup\$ – Anonymous Aug 6 '16 at 0:38
2
\$\begingroup\$

Your code looks promising but has some code smells.

  1. When you have a big method you should consider splitting the method up into methods/classes.
  2. When you have a lot of code duplication you should consider extracting common logic into methods/classes.
  3. Your code is hard to test, consider extracting related logic into classes.

Example of a refactoring that could help you clean this code:

public class String_Hash_Generator  {

    public static void main(String args[]) throws NoSuchAlgorithmException {
        Scanner inputScanner = new Scanner(System.in);
        System.out.print("String: ");
        String input = inputScanner.next();

        CryptographyGenerator generator = new CryptographyGenerator();

        System.out.println("\nMD2: " + generator.generateMD2(input)
                + "\nMD5: " + generator.generateMD5(input)
                + "\nSHA-1: " + generator.generateSHA1(input)
                + "\nSHA-256: " + generator.generateSHA256(input)
                + "\nSHA-384: " + generator.generateSHA384(input)
                + "\nSHA-512: " + generator.generateSHA512(input));
    }
}

public class CryptographyGenerator {

    public String generateMD2(String input) throws NoSuchAlgorithmException {
        return generateString(input, "MD2", 32);
    }

    public String generateMD5(String input) throws NoSuchAlgorithmException {
        return generateString(input, "MD5", 32);
    }

    public String generateSHA1(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-1", 40);
    }

    public String generateSHA256(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-256", 64);
    }

    public String generateSHA512(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-512", 128);
    }

    public String generateSHA384(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-384", 96);
    }

    private static String generateString(String input, String algorithm, int minLength) throws NoSuchAlgorithmException {
        MessageDigest messageDigest = MessageDigest.getInstance(algorithm);
        byte[] bytes = messageDigest.digest(input.getBytes());
        BigInteger integer = new BigInteger(1, bytes);
        String result = integer.toString(16);
        while (result.length() < minLength) {
            result = "0" + result;
        }
        return result;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Having all those specialized functions seems silly to me, when all they do is pass the algorithm name to the underlying function. If you're worried about typos in the algorithm name, I'd use named constants. \$\endgroup\$ – CodesInChaos Aug 8 '16 at 7:21
  • \$\begingroup\$ @CodesInChaos: I think the only worry is not just typos, but also enforcing the right minlength for each algorithm. \$\endgroup\$ – Attilio Aug 8 '16 at 7:59
0
\$\begingroup\$

First factor out the hex encoder into a function:

static String ToHex(byte[] bytes)
{
    BigInteger bigInt = new BigInteger(1, bytes);
    String s = bigInt.toString(16);
    while (s.length() < bytes.length() * 2) {
        s = "0" + s;
    }
}

How to convert a byte array to a hex string in Java? lists a number of alternative/better implementations.


You can handle all the different hashes in a loop, something like:

foreach(String algorithmName in new[]{"MD2", "MD5",...})
{
    MessageDigest hasher = MessageDigest.getInstance(algorithmName);
    byte[] hashBytes = hasher.digest(input.getBytes())
    String hashString = ToHex(hashBytes);
    System.out.println(algorithmName+": "+hashString);
}

input.getBytes() is dubious because it uses the locale dependent legacy encoding. It's generally better to use a fixed unicode encoding, preferably UTF-8. But in this case I see no reason to read the input as string instead of raw bytes in the first place.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.