Loading ...
Sorry, an error occurred while loading the content.

Refactoring Challenge: Michael Hill’s "Double-Dog-Dare" on an XWork class

Expand Messages
  • JeffGrigg
    Michael Hill issued a daring Double-Dawg-Dare for people to give him something significant to refactor as an example. He accepted the nomination of the
    Message 1 of 4 , Jan 10, 2010
    View Source
    • 0 Attachment
      Michael Hill issued a daring "Double-Dawg-Dare" for people to give him something significant to refactor as an example. He accepted the nomination of the AnnotationValidationConfigurationBuilder class in the opensymphony XWork library, and is in the process of refactoring it, and publishing the refactoring steps in screen casts. It should be interesting!

      The screencasts:
      http://anarchycreek.com/doubledawgdare-series/

      "Before and after" versions of his code are also available there.
      _ _ _

      To assist with this effort, I spent a day writing JUnit tests for the class.
      See "JUnit for MichaelHill DoubleDawgDare Refactoring.zip" file at
      http://tech.groups.yahoo.com/group/extremeprogramming/files/

      (I downloaded and built all the code from http://www.opensymphony.com/xwork/ But I found that the existing tests did not offer good coverage of the class we are refactoring.)
      _ _ _

      See also:
      http://tech.groups.yahoo.com/group/extremeprogramming/message/152504
    • JeffGrigg
      (Inspired by post rooted at... http://tech.groups.yahoo.com/group/testdrivendevelopment/message/32194 )
      Message 2 of 4 , Jan 10, 2010
      View Source
      • 0 Attachment
      • JeffGrigg
        Results uploaded to MichaelHill DoubleDawgDare Refactoring Challange Jeff Grigg.zip file on the XP discussion list s Files section:
        Message 3 of 4 , Jan 10, 2010
        View Source
        • 0 Attachment
          Results uploaded to "MichaelHill DoubleDawgDare Refactoring Challange Jeff Grigg.zip" file on the XP discussion list's Files section:
          http://tech.groups.yahoo.com/group/extremeprogramming/files/

          Add these files to Michel Hill's original (or later) solution to get my refactored solution (and a few updated tests).
          _ _ _

          Writing JUnit tests for the AnnotationValidationConfigurationBuilder class took all day and was tedious. But actually, it was not as hard as I thought it would be. And having the tests gives me an extremely valuable "safety net" that enables me to refactor the code and to have confidence that it still works. Without automated tests, I would have to do lots of manual testing. And I would have to do it over and over again, as I did the refactoring. So the cost of building the automated JUnit regression tests quickly pays for itself by saving manual testing effort.
          _ _ _

          So to get onto the refactoring...

          I don't like to pass two fields around that come from and represent a single object. So I changed "String fieldname, String methodName" to "Method method". I had to add local static "null safe" functions to avoid null deference issues.

          Working with one annotation class at a time, I moved all the code into subclasses of a new class in a new package:
          com.opensymphony.xwork2.validator.annotations.builders.AnnotationValidationBuilder

          For each annotation class I touched, I had to change code in two places:
          "private void processSingleAnnotation(List<ValidatorConfig> result, Annotation annotation, Method method)"
          and
          "private void processValidations(List<ValidatorConfig> result, Validations validations, Method method)"
          So I changed "processValidations" to delegate all of its work to
          "private List<ValidatorConfig> processAnnotationsArray(Annotation[] annotations, Method method)"
          Turns out nested arrays of annotations (lists of annotations specified in other annotations) can't be null, so I dropped all the null checks and did not add one to "processAnnotationsArray".

          So now my "processValidations" method is just 14 calls to "processAnnotationsArray".

          I got everything except Validation and Validations annotations moved over and then, to do them, added another layer of abstract base classes. Having moved them over, I realized that the remaining functionality in AnnotationValidationConfigurationBuilder looked a lot like the Validation/Validations code. So I created "ClassAnnotation..." and "MethodAnnotation..." classes, leading to the final state where the AnnotationValidationConfigurationBuilder class is only 31 lines long (including blank lines).

          The largest class in my solution is MultipleAnnotationValidationBuilder at 131 lines. And it's only that long because I refrained from doing reflection, so it has a large method with lots of if-then-else dispatching logic. Excluding that class, DateRangeFieldValidatorBuilder is the largest, at 92 lines. It takes a few lines to parse dates in three different formats.

          The original AnnotationValidationConfigurationBuilder class is 827 lines long.

          I went down a few blind alleys and rethought some things. But refactorings are reversible, so I eventually got where I wanted to be.
        • JeffGrigg
          ... I thought I d mention why I think that introducing two dozen new classes makes the code any better: If inheritance and polymorphism were not involved,
          Message 4 of 4 , Jan 11, 2010
          View Source
          • 0 Attachment
            --- "JeffGrigg" <jeffgrigg@...> wrote:
            > Working with one annotation class at a time, I moved all
            > the code into subclasses of a new class in a new package:
            > com.opensymphony.xwork2.validator.annotations.builders
            > .AnnotationValidationBuilder

            I thought I'd mention why I think that introducing two dozen new classes makes the code any better: If inheritance and polymorphism were not involved, then I'd just be moving the mess around, and not fixing it.

            The main objective of introducing new classes was to be able to factor out the common code in the 15 methods that looked like this:

            private ValidatorConfig process{TypeOfValidation}ValidatorAnnotation({ValidatorAnnotationClass} v, String fieldName, String methodName) {
            String validatorType = "{ShortKeyStringForThisValidation}";

            Map<String, String> params = new HashMap<String, String>();

            if (fieldName != null) {
            params.put("fieldName", fieldName);
            } else if (v.fieldName() != null && v.fieldName().length() > 0 ) {
            params.put("fieldName", v.fieldName());
            }

            // { Optionally one or more lines kinda like the following, or more complex }
            params.put("{key}", v.{attribute}());

            validatorFactory.lookupRegisteredValidatorType(validatorType);
            return new ValidatorConfig.Builder(validatorType)
            .addParams(params)
            .addParam("methodName", methodName)
            .shortCircuit(v.shortCircuit())
            .defaultMessage(v.message())
            .messageKey(v.key())
            .build();
            }

            The parts of this template that change between validators are the indicated in {CurlyBraces} above. The challenge is that we cannot refactor behavior and inheritance into the annotation class (the first parameter) because Java annotation classes are interfaces, so they cannot contain code. So I introduced a new set of builder classes, as first class Java classes, so that I could use inheritance and polymorphism to vary the parts of the method template above that are different between the validation annotations.
          Your message has been successfully submitted and would be delivered to recipients shortly.