Crap code (was: Re: Contribution and question about getting involved in the project.)

[Jonathan Revusky-3]

Tim Pizey wrote:
 >Sreeni was one of the original authors, so has had over ten years of
 >leading the
 >category killer open source java parser.

Well, obviously, if you have such reverence for this, you pretty much
preclude the ability of getting into the code. Because, once you stop
looking at this through rose tinted glasses, surely you start noticing
one recurring fact about the javacc code.

It's basically crap.

I'm going to provide one example, but it's typical of a lot of the stuff
in there that I've been cleaning up (in KawaDD.)

Consider this source file:

https://javacc.dev.java.net/source/browse/javacc/src/org/javacc/parser/RStringLiteral.java?view=markup

Look at the method DumpDfaCode() that begins on line 534 and ends around
500 (!) lines later. Yeah, look at that mofo!

Now, just as an example, take a look at the if (i != 0) block that
begins on line 570. I'll reproduce it here:

        if (i != 0)
         {
            if (i == 1)
            {
               for (j = 0; j < maxLongsReqd - 1; j++)
                  if (i <= maxLenForActive[j])
                  {
                     if (atLeastOne)
                        ostr.print(", ");
                     else
                        atLeastOne = true;
                     ostr.print("long active" + j);
                  }

               if (i <= maxLenForActive[j])
               {
                  if (atLeastOne)
                     ostr.print(", ");
                  ostr.print("long active" + j);
               }
            }
            else
            {
               for (j = 0; j < maxLongsReqd - 1; j++)
                  if (i <= maxLenForActive[j] + 1)
                  {
                     if (atLeastOne)
                        ostr.print(", ");
                     else
                        atLeastOne = true;
                     ostr.print("long old" + j + ", long active" + j);
                  }

               if (i <= maxLenForActive[j] + 1)
               {
                  if (atLeastOne)
                     ostr.print(", ");
                  ostr.print("long old" + j + ", long active" + j);
               }
            }
         }

Just eyeballing that, don't many people here find that this emits a
certain smell? Notice how the block within the if and within the else
are almost identical, for example???

If so, you're right. What if I made the conjecture that the above code
can be expressed as:

        for (j = 0; j < maxLongsReqd; j++) {
            if (i != 0 && i <= maxLenForActive[j] +1
                && maxLenForActive[j] != 0) {
                if (atLeastOne) {
                    ostr.print(", ");
                }
                if (i!=1) {
                    ostr.print("long old" +j + ", ");
                }
                ostr.print("long active" + j);
                atLeastOne = true;
            }
         }

Maybe you wouldn't believe it.

Well, believe it.... :-)

Of course, the current KawaDD codebase expresses it a bit differently,
because I factored out a little convenience method called outputArgs()
that I reuse. Here is what I have in the current code I'm working on.

             for (j = 0; j < maxLongsReqd; j++) {
                 if (i != 0 && i <= maxLenForActive[j] +1
                   && maxLenForActive[j] != 0) {
                     if (i!=1) {
                         args.add("long old" + j);
                     }
                     args.add("long active" + j);
                 }
             }
             outputArgs(ostr, args);

Of course, elsewhere, I have a convenience method outputArgs() that I
reuse in similar spots. It looks like this:

     void outputArgs(PrintWriter ostr, List<String> args) {
         ostr.print("(");
         for (Iterator<String> it = args.iterator(); it.hasNext();) {
             ostr.print(it.next());
             if (it.hasNext()) {
                 ostr.print(", ");
             }
         }
         ostr.print(")");
     }

Basically, I simply build up the list of args and let a separate little
routine deal with the annoying detail of not outputting an extraneous
final comma in the list.

The damnedest thing about this is that all of this is going to melt away
when I finally manage to templatize everything, but I needed to clean up
some of this ostr.println() stuff in order to have something that I
could convert to a template, since the existing stuff is just too messy.
So I need to encapsulate these ostr.println() patterns and so on just to
be able to get rid of the darned things. Kind of paradoxical.

I have a point here. The original code snippet I showed you is crap
code. Now that I am not trying to get involved in this project or
collaborate with anybody here, I don't have to mince my words. But this
is not an isolated thing. THere are pages and pages of this stuff in the
javacc codebase -- obviously, written by a newbie coder who did not
understand that copy-paste-modify is not a good way to write code.
Otherwise, you would think that by the nth time he coded the same
construct, he would look for a way to encapsulate it.

There's all kinds of horrendous stuff like this in the JavaCC codebase.
It needs a very serious cleanup. Well, I'm doing it and the project is
called KawaDD. But I am only part of the way there.  But okay, I said I
had a point. The real point here is that nothing can be done with this
without a huge code cleanup. You simply can't work with code like this.
It's not just that first horrendously verbose thing I showed, inside of
a 500 line method. The whole codebase is so entangled in so many ways,
with constant violation of basic principles of information
hiding/encapsulation, and DRY (don't repeat yourself.)

The code is largely horrendous, but it's there in that state year after
year. It is such a godforsaken mess that I actually empathize with
people who don't dare touch it... (Though, that said, obviously, any
non-trivial progress in this project has to involve touching it.)

I mean, this attitude of fawning reverence that Tim Pizey shows towards
JavaCC is completely misplaced. To do the kinds of cleanup I gave an
example of above does not require some kind of really superior knowledge
or talent. It's just a question of having the gumption and willingness
to do a certain amount of heavy lifting.

 >There is a difference in kind between the sort of tinkering that you,
I >and the others
 >are doing with Javacc and a change to the algorithms such that javacc
 >is on a
 >different mathematical footing.

Mathematical this, mathematical that.... yah dee dah dee dah....

The real real point here is that the reason that I can do some real work
of substance on this and you, in so many years, cannot, is not by dint
of any superior theoretical knowledge. Nah, it's because I have a
willingness to jump in there and get my hands dirty. That's the basic
reality of it. The rest is bullshit.

Of course, to drive away the only person who shows up in 5 years willing
to get his hands dirty.... well, I've already said enough about that...

The parser generation code is templatized by the way. I'm working on the
lexer generation now. If anyone wants to see what the parser generation
code looks like, it's actually in 2 files.

http://code.google.com/p/kawadd/source/browse/trunk/kawadd/src/kawadd/output/Parser.java.ftl
http://code.google.com/p/kawadd/source/browse/trunk/kawadd/src/kawadd/output/javacode.ftl

These things can be gradually cleaned up from here, of course, because a
lot of it is a bloody-minded port of what was in the
ostr.println()-based stuff before. Repeated patterns can be encapsulated
out to macros and so on.

But, the other aspect of this is that once you get all that
ostr.println() stuff out of the java code, it starts looking palatable.
For example, compare this:

http://code.google.com/p/kawadd/source/browse/trunk/kawadd/src/kawadd/parsegen/ParserData.java

to this:

https://javacc.dev.java.net/source/browse/javacc/src/org/javacc/parser/ParseEngine.java?view=markup

and this:

https://javacc.dev.java.net/source/browse/javacc/src/org/javacc/parser/ParseGen.java?view=markup

Regards,

Jonathan Revusky
--
KawaDD Parser Generator http://code.google.com/p/kawadd
lead developer, FreeMarker project, http://freemarker.org/


---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@...
For additional commands, e-mail: users-help@...

[Remi Koutcherawy]

Hi,
sorry I wonder disagreed.

Le 15/06/2008 00:54, Jonathan Revusky a écrit :
> Now, just as an example [...]
> Just eyeballing that, don't many people here find that this emits a
> certain smell? Notice how the block within the if and within the else
> are almost identical, for example???
Tout eyeballing que, ne pas beaucoup de gens ici ce que certains émet
une odeur?
Remarquez comment le bloc dans le si et dans l'autre sont presque
identiques, par exemple?

If I cannot understand your comment, at least I can understand the Java
code you posted.
The fact that the code is readable by a non English native is a good point.
But I am not sure this is what you meant to show.
> http://code.google.com/p/kawadd/source/browse/trunk/kawadd/src/kawadd/output/Parser.java.ftl 
>
This is not java, I shall need time to understand this new langage.
Please do not use it.

Remi Koutcherawy




---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@...
For additional commands, e-mail: users-help@...

[J.Chris Findlay]

On 6/15/08, Remi Koutcherawy <remi.koutcherawy@...> wrote:

>  If I cannot understand your comment, at least I can understand the Java
> code you posted.
>  The fact that the code is readable by a non English native is a good point.
>  But I am not sure this is what you meant to show.
>
> >
> http://code.google.com/p/kawadd/source/browse/trunk/kawadd/src/kawadd/output/Parser.java.ftl
> >
>  This is not java, I shall need time to understand this new langage.
>  Please do not use it.

The templating language in that file is freemarker, and allows [or
will allow] one to specify the output to be other languages than (or
variants of) java.

>  Remi Koutcherawy
> ---------------------------------------------------------------------
>  To unsubscribe, e-mail:
> users-unsubscribe@...
>  For additional commands, e-mail: users-help@...


--
 - J.Chris Findlay
   (c:

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@...
For additional commands, e-mail: users-help@...