Re: j2s-development] the state of the ASTScriptVisitor class

[Zhou R. <jo...@sj...>]

Hi Steven,

Thanks a lot for pointing out the nasty things! Thanks your advices.

It was my bad coding habit that made ASTScriptVisitor a mess. And this 
habit also affected other classes. And I will spend coming days to clean 
up those codes and add documents about ASTScriptVisitor and inner 
JavaScript simulator mechanics.

If there are other problems about the existed codes (Sure, there are 
lots of nasty codes), please feel easy to post comments to point them 
out. And I will try my best to clean things up.

Thanks.

Regards,
Zhou Renjian

Steven Devijver wrote:
> 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
>
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> j2s-development mailing list
> j2s...@li...
> https://lists.sourceforge.net/lists/listinfo/j2s-development
>
>
>