Question

Given: A LegacyControllerClass that extends a MonsterFrameworkClass (part of a very yucky framework that people are just living with for years). Framework class does lots of magic ranging from tons of logic in the default constructor to static blocks that reflectively load classes.

Lots of life-cycle methods in LegacyControllerClass, which mutate global states. The execute() method is a thousand-liner that has all the evils that you can think of.

public class LegacyControllerClass extends MonsterFrameworkClass {

   //Life-cycle method
   public void validate(){...}

   //Life-cycle method
   public void preExecute() {...}

   //Life-cycle method
   public void execute() {
       //oodles of legacy code here:
       //    examples: call static methods of 'Util' classes
       //              lots of new X().y(..)
       //              call databases, services
       //              Even call super.execute()!!
       //              More rubbish
       //              More rubbish 
   }
}

Okay, now the scene of action is the execute() method. I'm introducing Test-Driven-Development to these have-nots, by test-driving an item what they call as a 'story'. The 'story' involves adding an error message to the responseProperties so that the view(jsp) can read it off and display. The pseudo-code is something like:

if (error) {
    Build the error message
    Add the error message into the responseProperties
}

This code unfortunately has to be sand-witched between the rubbish in the execute() method.

My question is: what is the best thing I can do? The solution I could come up is:

  1. extracting two methods rubbish1() and rubbish2()
  2. stub them out in my test code with whatever expectations (Eg;- set the error flag)
  3. Put my code between rubbish1() and rubbish2()

I started implementing it this way, but the MonsterFrameworkClass is really getting in my way: like static loads, constructor magic that loads up a ResourceBundle of random properties files, etc.

Are there alternate ways of dealing with the same?

Teeny-tiny Disclaimer: I am definitely going to buy Michael Feather's "Working with Legacy Code" and gulp it down, but SO seemed to be a low-hanging fruit.

Was it helpful?

Solution

Refactoring the rubbish into methods is fine. Now you can download Powermock to mock out all the terrible rubbish in the code. After you are done with mocking you can test your code.

It is even better if you refrain from adding anything to this monstrosity. You can compose your functionality into MonstrosityRubbish (or whatever) by writing your own classes for the new stuff.

The main thing is not to touch any of the legacy code if possible, you can compose your code into it instead.

So in code:

private MyShinyClass yourShinyObject; // init

public void execute(Param param0) {
    rubbish1();
    yourShinyObject.handleError(param0);
    rubbish2();
}

public class MyShinyClass {

    public void handleError(Param param0) {
        // your code here
    }
}

If you can manage to write your new stuff like this you will only depend on Param which can be mocked/stubbed and your code can be tested without setting your hair on fire.

It is even better if you can write it in a functional way so you don't have to manipulate responseProperties in your separated code:

public void execute(Param param0) {
    rubbish1();
    responseProperties.add(yourShinyObject.fetchErrors(param0));
    rubbish2();
}

public class MyShinyClass {

    public List<HopelessError> fetchErrors(Param param0) {
        // your code here
    }
}

Of course execute() does not need a parameter you can pass any variable/field you want to handleErrors().

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top