Nice design of my hash service class

Question

I would like to know, if my design of this class is good OOP. Should I implement for every hash algorithm a separate class? I'm asking this, because the HashService can be used with a KeyedHashAlgorithm which wouldn't work correctly?!

public class HashService : IDisposable
{
    public HashService(HashAlgorithm algorithm)
    {
        HashAlgorithm = algorithm;
        Encoder = Encoding.UTF8;
    }

    protected HashAlgorithm HashAlgorithm { get; set; }

    /// <summary>
    /// The size, in bits, of the computed hash code.
    /// </summary>
    public int HashSize
    {
        get { return HashAlgorithm.HashSize; }
    }

    /// <summary>
    /// Gets or sets the encoding for strings.
    /// </summary>
    public Encoding Encoder { get; set; }

    public string ComputeHash(string input)
    {
        byte[] bytes = Encoder.GetBytes(input);
        byte[] hash = ComputeHash(bytes);
        return ToHex(hash); // method impl. omitted
    }


    /// <summary>
    /// Computes the hash value for the specified byte array
    /// </summary>
    /// <param name="buffer">The input to compute the hash code for.</param>
    /// <returns>The computed hash code.</returns>
    public byte[] ComputeHash(byte[] buffer)
    {
        return HashAlgorithm.ComputeHash(buffer);
    }

    public byte[] ComputeHash(byte[] buffer, int offset, int count)
    {
        return HashAlgorithm.ComputeHash(buffer, offset, count);
    }

    public byte[] ComputeHash(Stream inputStream)
    {
        return HashAlgorithm.ComputeHash(inputStream);
    }

    #region Implementation of IDisposable

    // omitted

    #endregion

    // ReSharper disable InconsistentNaming
    public static HashService CreateMd5()
    {
        return new HashService(new MD5CryptoServiceProvider());
    }

    public static HashService CreateRIPEMD160()
    {
        return new HashService(new RIPEMD160Managed());
    }

    public static HashService CreateSHA256()
    {
        return new HashService(new SHA256Managed());
    }

    public static HashService CreateSHA384()
    {
        return new HashService(new SHA384Managed());
    }

    public static HashService CreateSHA512()
    {
        return new HashService(new SHA512Managed());
    }

    public static HashService CreateSHA1()
    {
        return new HashService(new SHA1Managed());
    }

    public static HashService CreateCrc32()
    {
        return new HashService(new Crc32Managed());
    }

    // ReSharper restore InconsistentNaming 
}

Answer 1

Overall, its nice code. Easy to read and good use of white space.

I would move the creating of the HashService to a Factory class though. The way you are using it, I don't think it belongs in the class.

public class HashServiceFactory
{
    public Create(HashAlgorithm algorithm)
    {
        return new HashService(algorithm);
    }
}

// Usage

var factory = new HashServiceFactory();

var sha256Managed = factory.Create(new SHA256Managed());
var md5CryptoServiceProvider = factory.Create(new MD5CryptoServiceProvider());

Answer 2

Personally I think that all properties (Getters/Setters) should be at the top of the class, before the constructor. I believe that also matches Microsoft's style guide.

I personally use this.Property, I think its much easier to read and tells you its a class member not a local variable (or static method).

HashAlgorithm = algorithm; -> this.HashAlgorithm = algorithm;

Also naming, those are acronyms so capitalize all the letters.

public static HashService CreateMd5() -> public static HashService CreateMD5()
public static HashService CreateCrc32() -> public static HashService CreateCRC32()