SmellyCode: A Classical Example of Non-Unittestable Code

I know there might be some degree of repetition in my posts when I speak about unit testing and mocking, but I always again come over similar problems in code I'm reviewing. Most often the core problem is that the responsibilities among different entities are not clearly separated which in turn makes testing complex or even impossible.

In the following, I demonstrate a classical example I recently found in our codebase. The requirement was that you should be able to validate an Account object, certify it's validity, and if everything is fine, send the user a confirmation mail. This is a small piece taken out of a larger requirement, but it is enough to understand the point.

Original, smelly code
Roughly, the code has been structured as follows:



Does something attract your attention? No? Well, think about how to unit test the SaveAndCertifyAccount(..) method. Remember the requirement: first validate and persist, then certify and finally submit a confirmation mail. The diagram above shows all of the involved classes, where on the AccountBl side, SendConfirmationMail is private. This should recall your attention as this would mean that email sending is done directly there!

public void SaveAndCertifyAccount(Account anAccount)
{
   //some validation logic
   ...
   certificationBl.CertifyAccount(anAccount)
   if(anAccount.IsSuccessfullyCertified)
   {
      //prepare from, to, subject and body
      ...
      ExecuteEmailSending(from, to, subject, body);
   }
}

private void ExecuteEmailSending(string from, string to, string subject, string body)
{
    MailMessage mail = new MailMessage();
    mail.To.Add(to);
    mail.From = new MailAddress(from);
    mail.Body = body;
    mail.Subject = subject;

    SmtpClient aSmtpClient = new SmtpClient(System.Configuration.ConfigurationSettings.AppSettings["SmtpAdress"]);
    aSmtpClient.Send(mail);
}

This is more or less the "pseudocode" of the logic.

Objections and Comments
The highlighted lines pose the actual problem because

1. first of all they violate the "rule" of what is a unit test and what not (network access!!) and
2. they violate the S of the SOLID principles, namely: single responsibility (too much responsibility handled by the AccountBl class).

Proposed Refactoring
A more appropriate system would therefore look as follows:

---

Questions? Thoughts? Hit me up on Twitter