--- Victor Mote <[EMAIL PROTECTED]> wrote:
> I think it is instructive that neither of you
> commented on whether the
> changes that were made actually improved the code or
> not. 
>
> I think you both
> need to either 1) show that the code was better
> before I changed it, 

If the reason you gave in CVS for changing the code
was just to make it more readable or usable, then it
wouldn't have been an issue.  But the reason you gave
CVS for the change was primarily to make checkstyle
happy:

"extract methods nextDecimalPoint() and nextColor()
from method next(), primarily to
satisfy checkstyle's method size requirement"

That was my concern--a too-low limit requiring us to
rewrite a lot of code.  But according to your
response, you--not just checkstyle!--also happen to
feel it is a better design, so it's OK to keep as-is.

However, these changes do have a drawback because 
they encourage the use of "global" variables to avoid
passing parameters from one function to another.  For
example, it appears that currentTokenStartIndex is
actually just a throwaway local method variable so it
should not be declared at class-scope but as local
instead. But that's more difficult now because the two
functions which use cTSI have been broken out of
next()--you would need to pass cTSI as an argument
between those two.

(Also, as an aside, currentMaybeOperator isn't being
used at all, so in such a performance-critical area as
here perhaps the variable should be removed.)

Glen

__________________________________
Do you Yahoo!?
SBC Yahoo! DSL - Now only $29.95 per month!
http://sbc.yahoo.com

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, email: [EMAIL PROTECTED]

Reply via email to