j2s-development] the state of the ASTScriptVisitor class

[Steven D. <ste...@gm...>]

All,

I was planning to write my own ASTVisitor extension for j2s. The best
way to get started for me seemed to look at the existing visitor
classes. Thanks to the tutorial here
http://j2s.sourceforge.net/articles/tutorial-extended-compiler.html it
was easy to find the right class to look at: ASTScriptVisitor.

Sadly I soon found out this class is in decay. It's a whooping 3430
lines long (that's about 3000 lines in excess) and my guess is that
about one in every 5 lines of code is commented out. As if this is not
bad enough it gets worse.

There are many very very deeply nested code blocks that make this
class literally Chinese to anyone who wants to get to understand it.
In fact, although I have quite some experience in performing code
reviews I've never seen any class as poorly written as
ASTScriptVisitor. And mind you, this is the first and only class I
looked at in the entire j2s code base.

Now, you may think I'm ranting. Consider this. I've selected one
deeply nested code block out of many. This is its level of nesting.
This code block that starts on line 2234 in ASTScriptVisitor is nested
in:

an else block in a while loop in an if block in an if block in an if
block in an else block in an if block in an else block in a while loop
in an if block in an if block in an if block in an else block in an
else block in an if block in a method body.

That's 16 (!!!) levels of nesting without encapsulation!!! And again,
this is one example in a huge class in a big project.

My recommendations are simple:

- check in everything and create a tag in SVN today.
- stop adding new features today so that the project is frozen. Don't
add any new feature before the cleanup is complete.
- in every class of the project, remove all code that is commented
out. The only comments that are allowed are real comments in english,
no code. Start with the classes that have the most lines and continues
until you have checked every class in the codebase.
- test all features of your project and make sure your code works as
expected. Document exactly how you've tested every part of your code
base. Create an issue for every bug you come across that's likely
related to code that has been commented out.
- at this point, stop making any modifications to your code base.
- get together with all developers - either online to in real life -
and start discussing the state of the code base. Start with the
classes that have the most lines (the number of lines need to be
recounted after all commented out code has been removed). Discuss why
each class is as long as it is. Try to find out why these classes are
so long. Try to come up with abstractions and encapsulations. Create
some prototypes and verify if they are up to the job. If not improve
them or come up with alternatives. If you found something interesting
try it out on your code and see if it can improve the expressiveness
of your code.
- once you've selected a couple of code abstractions DOCUMENT THEM!!!
- now attack your code base and implement the abstractions you''ve
selected. Document each abstraction you implement like: ThisClass,
line x to y, ThisAbstraction, This and That class created.
- Re-test your codebase using the test descriptions you've written
down before. Create an issue for every bug your discover that wasn't
reported before.
- fix the issues that were discovered after adding the abstractions.
- now fix the issues that were discovered after the first test run.

It will require some time but not more that one or two months. Anyway,
if you don't do exactly as I've described your project is lost beyond
repair and you'd better stop working on it today.

Hope this helps. If you have any questions please feel free to contact me.

Steven Devijver