# Computing a hash value of a file

I am trying to see if there is a more efficient way of creating a hash of a file of unknown size (could be gb). My code below works fine but I am sure there is a more efficient way of achieving this.

    private static string GetHash(FileInfo fileInfo, HashAlgorithm algorithm)
{
byte[] Block = ArrayPool<byte>.Shared.Rent(8192);
string hashString = string.Empty;
try
{
using (algorithm)
{
{
int length;
while ((length = stream.Read(Block, 0, Block.Length)) > 0)
{
algorithm.TransformBlock(Block, 0, length, null, 0);
}
algorithm.TransformFinalBlock(Block, 0, 0);
hashString = algorithm.Hash.ToHex();
algorithm.Dispose();
stream.Flush();
}
}
fileInfo = null;
return hashString;
}
finally
{
ArrayPool<byte>.Shared.Return(Block);
}
}

private static string ToHex(this byte[] bytes, bool upperCase = true)
{
StringBuilder result = new StringBuilder(bytes.Length * 2);

foreach (var bit in bytes)
{
result.Append(bit.ToString(upperCase ? "X2" : "x2"));
}
bytes = null;
return result.ToString();
}


And the usage would be:

GetHash(fileInfo, SHA512.Create());

• To me fileInfo = null; is a bad practice. Don't mess with the input. – paparazzo Dec 19 '17 at 16:55
• Since there is no .net-core tag I think you should mention that you are using .net-core in your question. – t3chb0t Dec 19 '17 at 17:21
• algorithm.Dispose() in the middle is unnecessary. The using takes care of that for you. – Jesse C. Slicer Dec 19 '17 at 18:34
• more efficient way of creating a hash The code doesn't specify what the hash will/shall be good for. If it was just get something fixed length with collision rate unlikely to be much higher than hardly avoidable, use a less computationally intensive hash like Fletcher's. – greybeard Dec 23 '17 at 19:02

• Disposing algorithm is somewhat surprising - it's an argument, so I'd expect the calling code to be responsible for it. If anything, such behavior should be documented. You may also want to provide an overload that allows calling code to disable this - for example, StreamReader and StreamWriter have a constructor that takes a leaveOpen parameter.
• Don't call Dispose when you're already using a using statement.
• Successive using statements are usually not nested. Personally I think nesting them makes code a little less readable.
• stream.Flush() isn't necessary because you're not writing to the stream.
• Setting those local variables to null does nothing useful. It doesn't trigger the garbage collector, those variables will go out of scope shortly afterwards anyway, and since they're parameters they're also most likely not the only reference to the object they're referencing.
• After removing those unnecessary lines, you can immediately return the result of algorithm.Hash.ToHex() - no need to store it in a local variable. If you do want to use a variable (to ease debugging or to improve readability of the code), then I would declare it as close to where it's used as possible. Currently, it appears at the top, middle and near the end of the method, so you have to look all over the place to see how it's used. That's no big deal for a small method like this, but it's not as easy to understand as it could be.