Glen Mazza wrote: > 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"
OK, if I had known I would spend my weekend defending this, I would have clarified it with something like "primarily done right now (as opposed to later) 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. You need to draw a conclusion here. If you conclude that it was better before, you have my blessing to revert the change. In any other case, I will simply say thank you for the refresher course on the tradeoffs between local/global, number of parameters, and method size. > (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.) Please feel free to improve it further. Victor Mote --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, email: [EMAIL PROTECTED]
