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
SendConfirmationMailis private. This should recall your attention as this would mean that email sending is done directly there!
public void SaveAndCertifyAccount(Account anAccount)This is more or less the "pseudocode" of the logic.
//some validation logic
//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.From = new MailAddress(from);
mail.Body = body;
mail.Subject = subject;
SmtpClient aSmtpClient = new SmtpClient(System.Configuration.ConfigurationSettings.AppSettings["SmtpAdress"]);
Objections and Comments
The highlighted lines pose the actual problem because
- first of all they violate the "rule" of what is a unit test and what not (network access!!) and
- they violate the S of the SOLID principles, namely: single responsibility (too much responsibility handled by the
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.
A more appropriate system would therefore look as follows:
AccountBl.SaveAndCertifyAccount(...)can now be easily tested by mocking out the dependent classes.