Java: Best practices for turning foreign horror-code into clean API...?

Question

Hello, everyone!

I have a project (related to graph algorithms). It is written by someone else.

The code is horrible:

• public fields, no getters/setters
• huge methods, all public
• some classes have over 20 fields
• some classes have over 5 constructors (which are also huge)
• some of those constructors just leave many fields null
  (so I can't make some fields final, because then every second constructor signals errors)
• methods and classes rely on each other in both directions

I have to rewrite this into a clean and understandable API.

Problem is: I myself don't understand anything in this code.

Please give me hints on analyzing and understanding such code.

I was thinking, perhaps, there are tools which perform static code analysis and give me call graphs and things like this.

Answer 1

Well, the clean-up wizard in eclipse will scrape off a noticable percentage of the sludge.

Then you could point Sonar at it and fix everything it complains about, if you live long enough.

Answer 2

WOW!

I would recommend: write unittests and then start refactoring

* public fields, no getters/setters

start by making them private and 'feel' the resistance on compiler errors as metric.

* huge methods, all public

understand their semantics, try to introdue interfaces

* some classes have over 20 fields

very common in complex appilcations, nothing to worrie

* some classes have over 5 constructors (which are also huge)

replace them by by buider/creator pattern

* some of those constructors just left many fields null

see above answer

* methods and classes rely on each other in both directions

decide whether to to rewrite everything (honestly I faced cased where only 10% of the code was needed)

Answer 3

For static analysis and call graphs (no graphics, but graph structures), you can use Dependency Finder.

Answer 4

The answer may be: patience & coffee.

Answer 5

This is the way I would do it:

1. Start using the code , e.g. from within a main method, as if it were used by the other classes - same arguments, same invocation orders. Do that inside a debugger, as you see each step that this class makes.
2. Start writing unit tests for that functionality. Once you have reached a reasonable coverage, you will start to notice that this class probably has too many responsibilities.

while ( responsibilities != 1 ) {

1. Extract an interface which expresses one responsibility of that class.
2. Make all callers use that interface instead of the concrete type;
3. Extract the implementation to a separate class;
4. Pass the new class to all callers using the new interface.

}

Answer 6

Not saying tools like Sonar, FindBugs etc. that some have already mentiones don't help, but there are no magic tricks. Start from something you do understand, create a unit test for it and once it runs green start refactoring piece by piece. Remember to mock dependencies as you go along.

Answer 7

Oh dear :-) I envy you and not at the same time..ok let's take one thing at a time. Some of these things you can tackle yourself before you set a code analyzing tool loose at it. This way you will gain a better understanding and be able to proceed much further than with a simple tool

• public fields, no getters/setters
  • make everything private. Your rule should be to limit access as much as possible
• huge methods, all public
  • split and make private where it makes sense to do so
• some classes have over 20 fields
  • ugh..the Builder pattern in Effective Java 2nd Ed is a prime candidate for this.
• some classes have over 5 constructors (which are also huge)
  • Sounds like telescoping constructors, same pattern as above will help
• some of those constructors just left many fields null
  • yep it is telescoping constructors :)
• methods and classes rely on each other in both directions
  • This will be the least fun. Try to remove inheritance unless you're perfectly clear it is required and use composition instead via interfaces where applicable

Best of luck we are here to help

Answer 8

Use an IDE that knows something about refactoring, like IntelliJ. You won't have situations where you move one method and five other classes complain, because IntelliJ is smart enough to make all the required changes.

Unit tests are a must. Someone refactoring without unit tests is like a high-wire performer without a safety net. Get one before you start the long, hard climb.

Answer 9

Sometimes it is easier to rewrite something from scratch. Is this 'horrible code' working as intended or full of bugs? It is documented?

In my current project, deleting my predessor's work nearly in its entirety, and rewriting it from scratch, was the most efficient approach. Granted, this was an extreme case of code obfuscation, utter lack of meaningful comments, and utter incompetence, so your mileage may vary.