Better living through (unit) testing and advice

A few weeks ago I (accidentally) found out that a pet-project of mine, the cross-lingual instant messaging, wasn't working anymore. Debugging showed the "3rd party translation sub-system" was always returning an empty translation.

The fix revealed my development and maintenance process was flawed:

1. It was time consuming to identify the root cause;
2. My unit tests didn't address that particular scenario;
3. I wasn't notified when the problem first occurred, and so might have been unaware of the existence of a problem for God knows how long;
4. There was no user friendly message given to the user, who was left with emptiness where a translated message was expected;
   

The course of action required addressing each of the above listed issues:

1. Identify and fix the class of problems observed;
2. Improve prevention and identification of similar problems in the future, by expanding the coverage and improving the quality of the unit-tests;
3. Introduce a prevention and notification mechanism;
4. Improve the user experience by providing user-friendly error messages;
   

1) Identify and fix the class of problems observed
I just had to find out what changed in the 3rd party API and adapt my parser accordingly. Thanks to Spring, this involved a small change on a XML file and didn't require recompiling the code.
Now the next step forward is trying to automate this detection and adaption process. This is still work in progress to merit a post of its own.

2) Improve coverage and quality of unit-tests
Unit-tests that don't address particular scenarios are text-book cases of code smells. Here's the signature for my translation services:

public interface ITranslationEngine {
  /**
    * Translates 'text' from a given 'fromLanguage' into a given 'toLanguage'
    * @param fromLanguage
    * @param toLanguage
    * @param text
    * @return
    * @throws Exception
  */
  public String translate(String fromLanguage, String toLanguage, String text) throws TranslationException;
}

Implementations of the method translate above were responsible for the 3 tasks below:

1. send request to the 3rd party translation sub-system with the original message to translate, the source language, and the destination language;
2. fetch response from the translation sub-system;
3. parse response, extract and return translated string;
   

There weren't any unit-tests for any of these tasks because these multiple tasks were encapsulated under the one single method translate. When a method is long and/or does too much, it is harder to test. The method wasn't fine-grained enough to allow proper unit-test coverage, and there were at least two anti-patterns present in the tests:

• The One, where a unit-test contains one test method which tests an entire set of functionality;
• The Superficial Coverage, where exceptional conditions are missing from the test cases;
  

The fix involved extracting methods to simplify testing. The original translate method was refactored and each of the tasks listed above extracted into a well-named method. I added unit-tests that focused on each extracted method and considered input and output data in valid and exceptional scenarios. Loose coupling and testability go hand in hand.

3) Introduce a notification mechanism
To address this I implemented an Advice, a Spring schema-based AOP, to intercept the translate method, and check its arguments and return String. If the returned string is empty, a notification email is sent. The code snippet is shown below:

...
<aop:config>
   <aop:aspect id="translationEngineInterceptor" ref="translationEngineAdvice">
      <aop:after-returning
          method="notify"
          returning="translatedString"
          pointcut="execution(* translate(String, String, String)) and args(from, to, text)" />
   </aop:aspect>
</aop:config>

<bean id="translationEngineAdvice" class="org.jiim.translation.TranslationEngineAdvice">
    <property name="mailSender" ref="mailSender"/>
    <property name="mailMessage" ref="mailMessage"/>
</bean>

...

public class TranslationEngineAdvice {
  protected final Log logger = LogFactory.getLog(getClass());
  private MailSender mailSender;
  private SimpleMailMessage mailMessage;
  public static final String NEW_LINE = System.getProperty("line.separator");

  ...

  public boolean notifyByEmail(JoinPoint jp, Object translatedString, String from, String to, String text) {
    String s = (String) translatedString;
    // if message string returns empty translation
    if (!"".equals(text) && "".equals(s.trim())) {
      SimpleMailMessage msg = new SimpleMailMessage(this.mailMessage);
      StringBuffer txt = new StringBuffer(msg.getText());
      txt.append(NEW_LINE).append("Date [").append(new Date()).append("]");
      txt.append(NEW_LINE).append("From language [").append(from).append("]");
      txt.append(NEW_LINE).append("To language [").append(to).append("]");
      txt.append(NEW_LINE).append("Original Text [").append(text).append("]").append (NEW_LINE);
      msg.setText(txt.toString());
      try {
        this.mailSender.send(msg);
        return true;
      } catch (MailException me) {
        logger.error("Error sending email notification ", me);
      }
    }
    return false;
  }
}
...

4) Improve the user experience
I made a more considerate application by fetching a localized user friendly message to display if the translated message is empty.

References
Extracting Methods to Simplify Testing
Making Considerate Software
JUnit Anti-Patterns
TDD Anti-Pattern Catalogue
Reusable Advice - Part II: Unobtrusive Notification