Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 
}
share|improve this question
    
If you want to control the Algorithms supported then you could make the constructor private. Then you would control what algorithms you support. Not very OCP though??? –  dreza Jan 4 '13 at 0:57
add comment

2 Answers 2

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());
share|improve this answer
1  
In what regards is the way he using it make it better to put into a separate class? –  dreza Jan 4 '13 at 5:38
    
For one, it would allow addition of more HashAlgorithms without having to change the behaviour of HashService. –  Jeff Vanzella Jan 4 '13 at 17:13
1  
Agreed. Although he can still do that with the current implementation as the HashService constructor allows that. –  dreza Jan 4 '13 at 18:07
    
True. Even though its not shown in my example, with the factory, you could have all HashService instances you need in your app created in a static dictionary that has the HashAlogorithm as the key, change the Create method to accept a generic, or type, find that HashService in your dictionary and return it. Doing it this will would allow you to inject the factory and not have news all over your code. –  Jeff Vanzella Jan 4 '13 at 21:01
add comment

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()
share|improve this answer
    
No, MS spec is that properties come after constructors, but before other methods. –  Jesse C. Slicer Jan 4 '13 at 18:35
    
The MS style guide says that you should only capitalize the first letter of acronyms with at least three letters. "When using acronyms, use Pascal case or camel case for acronyms more than two characters long. For example, use HtmlButton or htmlButton. However, you should capitalize acronyms that consist of only two characters, such as System.IO instead of System.Io." –  CodesInChaos Feb 4 '13 at 16:03
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.