RE: Checkstyle max method length
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]
RE: Checkstyle max method length
--- 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]
Re: Checkstyle max method length
Victor Mote wrote: Peter B. West wrote: Glen Mazza wrote: Victor, I noticed we had to break up a function to satisfy Checkstyle's max method length size of 150 ( http://marc.theaimsgroup.com/?l=fop-cvs&m=105806596213734&w=2). That seems too constricted for our use--it's may force parameter passing of local variables where none would otherwise be needed. I would double the limit to 300--the function above did not warrant rewriting--a better option may be to remove the limit entirely. Combined, the two case statement blocks tripped Checkstyle's limit, causing Checkstyle to complain: so you placed both blocks into separate functions. But what if the second case statement was much smaller? Checkstyle wouldn't complain about the method containing on the switch statement, so *neither* of the two case statements would be placed into their own function. This seems too arbitrary: something in addition to method size should play a part in the decision to create new methods. Glen, Victor, I tend to agree with Glen on this one. The limit seems too restrictive, and it is only a guideline - as are many of the other strictures of Checkstyle. They flag things which may make the code problematical, but they do not have the same simplicity as, e.g., tab width or format of names. If we follow them slavishly, we lose perspective on the code. I don't think things like method length should even be warnings; rather info flags, to indicate that something needs to be looked at. 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, or 2) realize that checkstyle did what it was supposed to do by flagging based on an admittedly arbitrary limit. I will go one step further and suggest that each of you find one 150+ line method in FOP's source code that would not benefit from refactoring into smaller methods. The truth is that I too am uncomfortable with a 150-line limit, and will argue against it if I ever think it is a problem. The simple fact is that every 150+ line method I have encountered so far needed to be broken up. There was nothing slavish about it. Some of the ones in question will benefit from being broken up even further. Some other day. I didn't comment on the particulars because I was speaking generally about the way to react to the method length limit. I should have said that I was agreeing with Glen concluding remark, and I should have removed the rest of the posting. My apologies, Victor. Peter -- Peter B. West http://www.powerup.com.au/~pbwest/resume.html - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, email: [EMAIL PROTECTED]
RE: Checkstyle max method length
Peter B. West wrote: > Glen Mazza wrote: > > Victor, > > > > I noticed we had to break up a function to satisfy > > Checkstyle's max method length size of 150 ( > > http://marc.theaimsgroup.com/?l=fop-cvs&m=105806596213734&w=2). > > That seems too constricted for our use--it's may > > force parameter passing of local variables where none > > would otherwise be needed. I would double the limit > > to 300--the function above did not warrant > > rewriting--a better option may be to remove the limit > > entirely. > > > > Combined, the two case statement blocks tripped > > Checkstyle's limit, causing Checkstyle to complain: > > so you placed both blocks into separate functions. > > But what if the second case statement was much > > smaller? Checkstyle wouldn't complain about the > > method containing on the switch statement, so > > *neither* of the two case statements would be placed > > into their own function. This seems too arbitrary: > > something in addition to method size should play a > > part in the decision to create new methods. > > Glen, Victor, > > I tend to agree with Glen on this one. The limit seems too restrictive, > and it is only a guideline - as are many of the other strictures of > Checkstyle. They flag things which may make the code problematical, but > they do not have the same simplicity as, e.g., tab width or format of > names. If we follow them slavishly, we lose perspective on the code. I > don't think things like method length should even be warnings; rather > info flags, to indicate that something needs to be looked at. 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, or 2) realize that checkstyle did what it was supposed to do by flagging based on an admittedly arbitrary limit. I will go one step further and suggest that each of you find one 150+ line method in FOP's source code that would not benefit from refactoring into smaller methods. The truth is that I too am uncomfortable with a 150-line limit, and will argue against it if I ever think it is a problem. The simple fact is that every 150+ line method I have encountered so far needed to be broken up. There was nothing slavish about it. Some of the ones in question will benefit from being broken up even further. Some other day. Victor Mote - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, email: [EMAIL PROTECTED]
Re: Checkstyle max method length
Glen Mazza wrote: Victor, I noticed we had to break up a function to satisfy Checkstyle's max method length size of 150 ( http://marc.theaimsgroup.com/?l=fop-cvs&m=105806596213734&w=2). That seems too constricted for our use--it's may force parameter passing of local variables where none would otherwise be needed. I would double the limit to 300--the function above did not warrant rewriting--a better option may be to remove the limit entirely. Combined, the two case statement blocks tripped Checkstyle's limit, causing Checkstyle to complain: so you placed both blocks into separate functions. But what if the second case statement was much smaller? Checkstyle wouldn't complain about the method containing on the switch statement, so *neither* of the two case statements would be placed into their own function. This seems too arbitrary: something in addition to method size should play a part in the decision to create new methods. Glen, Victor, I tend to agree with Glen on this one. The limit seems too restrictive, and it is only a guideline - as are many of the other strictures of Checkstyle. They flag things which may make the code problematical, but they do not have the same simplicity as, e.g., tab width or format of names. If we follow them slavishly, we lose perspective on the code. I don't think things like method length should even be warnings; rather info flags, to indicate that something needs to be looked at. Peter -- Peter B. West http://www.powerup.com.au/~pbwest/resume.html - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, email: [EMAIL PROTECTED]
