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);
}
}
3 Answers 3
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
-
\$\begingroup\$ Using this, could I also delete the line
String hcMD2 = intNumMD2.toString(16);
? \$\endgroup\$Anonymous– Anonymous2016年08月06日 00:31:16 +00:00Commented Aug 6, 2016 at 0:31 -
\$\begingroup\$ Yes, I'll add that to the answer. \$\endgroup\$GiantTree– GiantTree2016年08月06日 00:31:44 +00:00Commented Aug 6, 2016 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– Anonymous2016年08月06日 00:38:29 +00:00Commented Aug 6, 2016 at 0:38
Your code looks promising but has some code smells.
- When you have a big method you should consider splitting the method up into methods/classes.
- When you have a lot of code duplication you should consider extracting common logic into methods/classes.
- 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;
}
}
-
\$\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– CodesInChaos2016年08月08日 07:21:13 +00:00Commented Aug 8, 2016 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– Attilio2016年08月08日 07:59:20 +00:00Commented Aug 8, 2016 at 7:59
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.
Explore related questions
See similar questions with these tags.
while
loops). Also: Don't useBigInteger
, it's painfully slow. Look at this answer: stackoverflow.com/a/473309/4867727 \$\endgroup\$