Checking an MD5 of a device

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())
{
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.

• You only posted the MD5 code but omitted the CRC code. – CodesInChaos Nov 18 '15 at 17:09
• 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. – Oscar Guillamon Nov 19 '15 at 6:55
• 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("-","") – CodesInChaos Nov 19 '15 at 7:42

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())
{

md5String = md5CRC.ToHex();
}

• 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 – Oscar Guillamon Nov 18 '15 at 12:19

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


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.

• 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 – Oscar Guillamon Nov 18 '15 at 14:06
• It seems you don't like returning null values, can you explain me why? – Oscar Guillamon Nov 18 '15 at 14:10
• @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. – RobH Nov 18 '15 at 14:13
• It makes sense you point of view, I'll keep it in mind when developing future code, thanks for the tip – Oscar Guillamon Nov 18 '15 at 14:15
• 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. – CodesInChaos Nov 19 '15 at 7:51