3
\$\begingroup\$

The point of this code snippet is to calculate and compare an MD5 obtained from a USB device connected to the computer against a database to check its validity.

var md5String = string.Empty;
using (var md5 = MD5.Create())
{
    var md5CRC = md5.ComputeHash(File.ReadAllBytes(destinationFilename));
    foreach (byte b in md5CRC)
        md5String += b.ToString("X2");
}

// here I check the MD5 against the database to check the usb device is valid

The code meets its purpose, but I want to know how I can improve this code.

\$\endgroup\$
  • \$\begingroup\$ You only posted the MD5 code but omitted the CRC code. \$\endgroup\$ – CodesInChaos Nov 18 '15 at 17:09
  • \$\begingroup\$ Yeah, you are right, I only posted the code regarding the MD5, because the engineer stated later that only the MD5 should be checked, it was a confusion on my part. \$\endgroup\$ – Oscar Guillamon Nov 19 '15 at 6:55
  • 1
    \$\begingroup\$ How do you convert Byte Array to Hexadecimal String, and vice versa? lists many different ways to convert bytes to hex. Personally I like BitConverter.ToString(bytes).Replace("-","") \$\endgroup\$ – CodesInChaos Nov 19 '15 at 7:42
5
\$\begingroup\$

Using string concatenation in a loop is a no go because each time you do a += a new string object will be created because strings are immutable.

To use the right tool for the job I would suggest to use a StringBuilder instead. Using a StringBuilder doesn't have this restriction because the passed strings are just appended char by char to the internal buffer.

Using braces {} although they might be optional will make your code less error prone.

By extracting the building of the hex string representation to a separate method the responsibility is split nicely and could be reused if needed. Having this as a extension method could serve well.

private string ToHex(this byte[] values)
{
    if (values == null) { throw new ArgumentNullException("values"); }

    StringBuilder sb = new StringBuilder(values.Length * 2);
    foreach (var b in values)
    {
        sb.Append(b.ToString("X2"));
    }

    return sb.ToString();
}  

resulting in the former code like this

var md5String = string.Empty;
using (var md5 = MD5.Create())
{
    var md5CRC = md5.ComputeHash(File.ReadAllBytes(destinationFilename));

    md5String = md5CRC.ToHex();
}
\$\endgroup\$
  • \$\begingroup\$ Yes, you are right, I forgot to use stringBuilder even though I've used it in the past, I guess coding in a hurry has its problems, marked as answer \$\endgroup\$ – Oscar Guillamon Nov 18 '15 at 12:19
4
\$\begingroup\$

ReadAllBytes loads the whole file into memory. That's prohibitive for large files and puts a lot of pressure on the GC even for moderate file sizes. I recommend hashing a stream instead.

byte[] HashFile(string filename, Func<HashAlgorithm> createHash)
{
    using (var stream = File.OpenRead(filename))
    using (var hasher = createHash())
    {
        return hasher.ComputeHash(stream);
    }
}

string ToHexString(byte[] bytes)
{
    return BitConverter.ToString(bytes).Replace("-", "");
}

Combining it with a ToHexString method, like @Heslacher recommended:

ToHexString(HashFile(filename, MD5.Create))
\$\endgroup\$
3
\$\begingroup\$

Helsacher (as usual) has provided an excellent answer but I'd like to suggest an alternative implementation to his extension method:

public static string ToHexString(this IEnumerable<Byte> bytes)
{
    if (bytes == null) 
    {
        // throw or possible return null if you're in to that sort of thing.
    }
    return string.Join(string.Empty, bytes.Select(b => b.ToString("X2")));
}

It operates on an IEnumerable<Byte> not just an array which makes it more generally usable. I also think using string.Join is more readable than a foreach loop but your mileage may vary.

\$\endgroup\$
  • \$\begingroup\$ This answer is quite brilliant too, I'll keep it on my library and use it depending on my needs, thanks as well for the help \$\endgroup\$ – Oscar Guillamon Nov 18 '15 at 14:06
  • \$\begingroup\$ It seems you don't like returning null values, can you explain me why? \$\endgroup\$ – Oscar Guillamon Nov 18 '15 at 14:10
  • \$\begingroup\$ @OscarGuillamon - It depends entirely on the situation - if null byte arrays are allowed in your code I think you should return null if not, I think you should throw an exception. If it were my own code, I would throw an ArgumentNullException as I would use an empty byte array to mean no data not null. \$\endgroup\$ – RobH Nov 18 '15 at 14:13
  • \$\begingroup\$ It makes sense you point of view, I'll keep it in mind when developing future code, thanks for the tip \$\endgroup\$ – Oscar Guillamon Nov 18 '15 at 14:15
  • \$\begingroup\$ In general I agree with the throwing when receiving null arguments, but with ToHexString it can be convenient to return null so you can visualize byte[] values without caring if they're null. \$\endgroup\$ – CodesInChaos Nov 19 '15 at 7:51

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.