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).
While you may still be able to automatically test your AccountBl class, it is not suitable for a unit test, as you would send an email each time you run your tests...
The more important however is point number 2: when thinking about a clear separation of responsibilities one immediately sees that the AccountBl's responsibility shouldn't be to send emails, but rather it should request "someone else" to perform that operation, i.e. an EmailSender or similar.

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

You can now see that the email sending functionality has been moved to a separate class EMailManager (abstracted through an interface). The program logic within AccountBl.SaveAndCertifyAccount(...) can now be easily tested by mocking out the dependent classes.
Kindle

Comments

0

Your ad here?