--- 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]
