Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have the following SHA256 wrapper.

    public static string SHA256(string plainText)
    {
        StringBuilder sb = new StringBuilder();

        SHA256CryptoServiceProvider provider = new SHA256CryptoServiceProvider();
        var hashedBytes = provider.ComputeHash(Encoding.UTF8.GetBytes(plainText));

        for (int i = 0; i < hashedBytes.Length; i++)
        {
            sb.Append(hashedBytes[i].ToString("x2").ToLower());
        }

        return sb.ToString();
    }

Do I want to be testing it? If so, what do you recommend?

My thought process is as follows:

  1. What logic is there here.
  2. The answer is my for loop and ToString("x2") so from my understanding I want to be testing this part?

I can assume Encoding.UTF8.GetBytes(plainText) works. Correct assumption? I can assume SHA256CryptoServiceProvider.ComputeHash() works. Correct assumption?

I want to be only testing my logic. In this case is limited to the printing of hex encoded hash. Correct?

Thanks.

EDIT:

Update method based on MainMa's excellent answer:

    /// <summary>
    /// Returns a SHA256 hash of the plain text.
    /// </summary>
    /// <param name="plainText">The pain text.</param>
    /// <returns>The hash.</returns>
    public static string SHA256(string plainText)
    {
        if (string.IsNullOrWhiteSpace(plainText)) throw new ArgumentNullException("The plain text cannot be empty.");

        StringBuilder sb = new StringBuilder();

        Byte[] hashedBytes;
        using (SHA256CryptoServiceProvider sha256Provider = new SHA256CryptoServiceProvider())
        {
            hashedBytes = sha256Provider.ComputeHash(Encoding.UTF8.GetBytes(plainText));
        }

        foreach(Byte b in hashedBytes)
        {
            sb.Append(b.ToString("x2").ToLower(CultureInfo.InvariantCulture));
        }

        return sb.ToString();
    }
share|improve this question
1  
This would be much easier to test if you made hashedBytes the parameter to the method, rather than plainText. The method, in it's current state, has more responsibilities than it needs to, which makes testing harder. –  Scroog1 Jul 2 '13 at 11:16
1  
Alas, I am unable to remove my erroneous apostrophe. Woe is me! –  Scroog1 Jul 2 '13 at 15:09
add comment

2 Answers 2

up vote 8 down vote accepted

Note: a more detailed discussion of the code is available at my website. It covers nearly all the points covered here, and shows, step by step, how this piece of code can be refactored into a piece of reliable software.


First, you may want to test the edge cases.

  • What if plainText is null? You don't check the inputs. Is this intentional, i.e. you rely on GetBytes to throw ArgumentNullException in your place? Is this a mistake?

  • What if plainText is long, and by long, I mean the maximum allowed by Windows? This is a complicated case, since unit testing it is not obvious (including in terms of performance and memory consumption: nobody wants their unit tests taking all memory available), but you still should consider it.

    Hint: the easiest and fastest way to handle this case is to specify the maximum supported length in the product requirements, and then enforce this limit at input validation step.

  • What if plainText is an empty string?

  • What if you are calling the method plenty of times? You don't dispose SHA256CryptoServiceProvider, despite the fact that you should, which means that sooner or later, it will fail more or less unpredictably. This is another case where unit testing is complicated/practically impossible.

Second, you're right, you can rely on the methods from .NET Framework and assume they are correctly implemented. This means that you have to test your logic and the way you use those methods:

  • Write a simple test which calls the method with "Hello World!" as input. Take any non-.NET language: JavaScript, Python, whatever. Compute the SHA256 hash with this language and hardcode the result in your unit test. Compare.

  • Analyse your code (note: in TDD, you don't do that: you write tests from the use cases, not the actual code). Such analysis should give you the overall view of the flow: where are the conditions, how do you exit the loops, are there guard clauses, etc. The actual flow is easy: a sequence of calls to different methods, then a for. Aside the missing using, the sequence of calls can create a risk of using a variable while it is null. For example:

    var product = this.GetProductById(productId);
    var category = product.GetCategory();
    

    is wrong, unless there is a guarantee that GetProductById cannot return null. The previous piece of code should have been:

    var product = this.GetProductById(productId);
    if (product == null)
    {
        // Handle the situation and either throw an exception or prematurely return/break.
    }
    
    var category = product.GetCategory();
    

    or:

    var product = this.GetProductById(productId);
    Debug.Assert(
        product != null,
        "The null result is unexpected from ProductsManager.GetProductById method.");
    
    var category = product.GetCategory();
    

    or:

    var product = this.GetProductById(productId);
    Contract.Assume(product != null);
    var category = product.GetCategory();
    

    In your case, there are only constructors, so you don't have to care about null references.

    for that you erroneously use (it should have been foreach) is another point to consider. Is the loop made correctly? Do you have to do i < hashedBytes.Length or i <= hashedBytes.Length? When you access the hashedBytes by index, are you sure that the index is never out of range? Could the length be bigger than int.MaxValue? What would happen?

    Finally, you're using both byte.ToString and ToLower without specifying the culture. Since you don't say neither that you're expecting the InvariantCulture to be used, it may indicate that you simply forgot to clearly indicate your intent. Both methods use the current culture. This means that you have also to unit test cases where the current culture is not en-US, since the final result of your SHA256 method depends on it (or use formal methods to show that the result would be the same for any culture).

    Moreover, byte.ToString("x2") already produces a lowercase string. Converting a lowercase string to lowercase is not needed. Always get rid of the code you don't need.

Conclusion:

Your assumption that you can rely on methods from .NET Framework is true. However, the way you use them should be unit tested. As you can see, in this very short method with simple logic,

  • there is a potential severe bug which will appear sooner or later when somebody will start to modify this method.

  • by not disposing objects properly, created a bug which will be very hard to debug later,

  • made it error prone by wrongly using for when foreach was expected,

  • made it potentially vulnerable by not checking your inputs (while this is a public method),

  • made it difficult to guess which exceptions may be thrown by this method,

  • made its behavior very random when it comes to edge cases (strings too long, null, etc.)

You could have avoided few of those issues with severe testing, but testing alone is not enough. Using other techniques, such as informal code reviews or static checking should be systematic and cover if not all, at least most non-CRUD code you write.

share|improve this answer
1  
+1 for beating me to the punch with a better, more complete answer. –  Ampt Jul 2 '13 at 12:16
    
Awesome, thanks a lot. –  Sam Leach Jul 2 '13 at 12:29
    
I've edit my question with the updated method. How does it look now? Is there anything in the production code that you would add to address your 'plainText is long' case? –  Sam Leach Jul 2 '13 at 12:54
1  
@SamLeach: I'm writing a more complete blog article about the question. Given my tight schedule, I can't give any delay, but I'll post a comment when finished. As for your actual revised code, the only note for now that I have is that ArgumentNullException constructor takes as parameter the name of the parameter, not the message. –  MainMa Jul 2 '13 at 22:12
1  
@SamLeach: I've finished the article. The link is available at the top of the answer. –  MainMa Jul 4 '13 at 21:01
show 3 more comments

In a simple function like this there aren't many tests as unit tests are defined by what output are you expecting the function to return.

You can assume that the built in functions work, but could any of those functions throw an exception based on the input you provide - is that something your function needs to handle?

Tests you might consider - is the resulting string the correct length (if someone comes along and switches SHA256 for another scheme? Are there failure states and how should the function respond to or report them?

share|improve this answer
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.