On Tue, August 9, 2005 1:04 am, Martin Cooper said: > You forgot FindBugs. It really does find bugs. ;-)
Your right, I did :) My builds at work do include it actually. > Checkstyle is run from Maven, so whether or not we move to Checkstyle > 3.5 right now depends on whether or not the Maven Checkstyle plugin > supports it. If it does, I'm fine with upgrading, assuming you can get > the new Checkstyle jar added to ibiblio. Good point... anyone know the answer to this? I guess subconsciously I had made the assumption that the plugin wouldn't care what version it was using, maybe that's not a safe assumption though. > As for adding more checks - given how many errors/warnings we have > now, I would be -1 on changes that generate more, until we seriously > reduce the current count. Once we're clean, or nearly so, I'd be OK > with selectively enabling more checks. Fair answer. If everyone else feels the same way I can move ahead with getting rid of as many as possible with the current ruleset. Frankly, most of them are things like tabs-to-spaces and simple javadoc fixups, so I can probably mow them down rather quickly. > Per Craig's comment, I think we'd want to see how much this finds that > we'd want to clean up, versus what we want to keep the way it is, and > if this can be fine-tuned. I'm very much opposed to having warnings > show up that we declare to be "OK", so the goal would have to be a > completely clean Checkstyle run rather than one that results in > warnings that "we can ignore". As another person pointed out, most of what this catches seems to be dealing with the license header... this check didn't seem to be as configurable as I would have liked, so I think in retrospect I'd probably withdraw this suggestion. I agree with your feeling that the goal should definitely be a completely clean run. That is always my goal with my own code. However, having worked with all these static analysis tools a fair bit, my experience tells me that no matter how you can or try to configure the rules, they sometimes flag things that either aren't actually problems (that's fairly rare actually) or flag things that you as the developer have a reason for having broken the rule. So, while 100% clean should definitely be the goal, 99% is OK in my opinion, so long as you can explain that 1%. >> * AbstractClassName > > We could try it out, but I seriously doubt that we could come up with > a pattern that would make this rule pass. ;-) And changing the class > names to make it work might well break backwards compatibility. I actually only count 9 such warnings, so it might not be as onerous as it seems... that's using the default match pattern, and certainly that could be tweaked. I too was concerned with breaking backwards-compatibility though, so certainly this has to be done with caution. Many of the rules result in things that can be changed with little or no risk, this probably isn't one of them. >> * ImportOrder > > I don't much care about this one. If a class has so many imports that > you can't see the wood for the trees, the class is probably due for > refactoring anyway. ;-) Fair enough :) It doesn't sound like your against it, jsut don't see much value in it... your probably right, aside from consistency I suppose, which should count for something I figure :) >> * StringLiteralEquality > > Sure. That's more a bug than a style issue anyway. Absolutely :) Many of the things CheckStyle can check for aren't actually style-related any more. None of these types of issues show up currently by the way, so this would just be a check for the future. >> * IllegalCatch > > I'm not sure if there are places in Struts where we do this and need > to do this. If not, then I'm fine with adding this check to make sure > that we don't do it in the future. If so, then we need to find a way > to avoid warnings in these particular cases. I count about 55 of these types of warning... I have not looked at the actual code at all though, so I don't know if they are legitimate rule-breakages or not. >> * PackageDeclaration > > Yes, this is silly. If Struts committers are checking in classes with > no package declaration, then I think we have bigger problems. ;-) No question :) I'm of the "no harm to foul" school of thought on checks like this though. >> * ExplicitInitialization > > I'm pretty sure that modern JVMs deal with the double init, so I don't > think there's any perf issue. Also, it's sometimes clearer for Java > newbies to see things initialised. However, I don't really care one > way or the other. I suspect your right about modern JVMs, I'd kind of be surprised if they didn't handle this. For a long time I always explicitly initialized everything... came from my C background I suppose. Then, I read something that changed my mind on this, and I wish I could remember where, when or who said it... "When you explicitly initialize variables to their default values, it shows that you don't trust the language". It's not much of a reason to not explicitly initialize I suppose, but for some reason if kind of struck a chord with me. Well, certainly this isn't any huge thing either way. >> * UnnecessaryParentheses > > I agree with Craig on this one, but I'm not sure exactly what the rule > will flag and not flag. I'd be fine if it flags gratuitously > unnecessary parentheses and leaves those that aid in clarity, but that > seems like a rather subtle semantic for a tool like Checkstyle to deal > with. ;-) In looking through the results, it looks like almost all of them in fact are parens around return values. I do agree that CheckStyle isn't going to be able to always properly determine when parens are really "superfluous" or not, although I think it uses the simple definition that if they aren't strictly needed to ensure proper order than they are superfluous. Clearly that doesn't take added clarity into account, and it can't, I agree. > Dang, I thought I caught all the tabs! I sure tried. Maybe people have > been adding them again... There's gotta be a Maven plugin for that, no :) > Martin Cooper Frank --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
