7
\$\begingroup\$

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)
            {
                using (FileStream stream = fileInfo.OpenRead())
                {
                    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());
\$\endgroup\$
  • \$\begingroup\$ To me fileInfo = null; is a bad practice. Don't mess with the input. \$\endgroup\$ – paparazzo Dec 19 '17 at 16:55
  • \$\begingroup\$ Since there is no .net-core tag I think you should mention that you are using .net-core in your question. \$\endgroup\$ – t3chb0t Dec 19 '17 at 17:21
  • 2
    \$\begingroup\$ algorithm.Dispose() in the middle is unnecessary. The using takes care of that for you. \$\endgroup\$ – Jesse C. Slicer Dec 19 '17 at 18:34
  • \$\begingroup\$ 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. \$\endgroup\$ – greybeard Dec 23 '17 at 19:02
7
\$\begingroup\$

I think your overall approach is sound: read data into a buffer, update the hash, and repeat until the whole file has been processed.

Buffer size may have some impact on performance, so that may be worth experimenting with (I'd probably expose it as a parameter).

Having that said, there are a few odd things going on in your code:

  • 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.
| improve this answer | |
\$\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.