Re: Code Quality Metrics
Max Berger wrote: Hi Max, snip/ Many of these could be automatically solved using the eclipse cleanup tools (which can actually be called on the whole src dir!). However, that would result in a change in almost every file, and making merging of separate branches almost impossible. This should therefore not be done until 0.95 is released, to allow backporting. The main question is, should it be done at all? I just wanted to remind you that the 0.95 release branch is not the only branch being actively worked on. For example; Adrian has 2 AFP branches that he is currently developing. Simon also has a branch with an alternative layout algorithm So doing a global cleanup of the code in trunk only would cause all sorts of problems. Chris snip/
Re: Code Quality Metrics
Hi, Jeremias Maerki wrote: I'm scared by the thought of having a program clean our source code changing lots of files. I prefer the approach that the devs shall try to improve the code while they are working on it. Agreed. Even if that doesn’t prevent us from scheduling a big “code cleanupathon” when we’re ready, i.e., at a moment where there’s no active branch, only the Trunk. Far, far away? Probably ;-) BTW, I think there's one or two rules in our Checkstyle file that probably should/could be removed. For example, the frequent warning about field hiding and Utility classes should not have a public constructor. But that's a separate discussion I guess. I’ve been wanting to lauch this discussion for a while. It definitely makes sense to re-visit all of the rules and decide upon a set that we really want to follow. That should encourage devs to take checkstyle warnings more into account, instead of just thinking: “That bl... checkstyle annoys me with all its stupid warnings”. I must confess that that’s what I’m thinking myself sometimes :-\ 18000 PMD violations is just sick. Things like rule [1] doesn't really help the source code. We can do that if we get a budget for nuclear-power-plant-grade software. Same here I guess. Now may be the right time to launch the debate, actually. I’ll try to gather some energy in the next days for that. [1] http://pmd.sourceforge.net/rules/optimizations.html#MethodArgumentCouldBeFinal +1 I must say, I’ve never really grasped the benefit of doing this. I’d be happy to be enlightened, though. On 12.06.2008 08:28:35 Max Berger wrote: Dear Fop-Devs, Jeremias is right - you actually need to use the output of these reports. At this time there are: 1849 checkstyle violations 18702 pmd violations possible (find)bugs. Many of these could be automatically solved using the eclipse cleanup tools (which can actually be called on the whole src dir!). However, that would result in a change in almost every file, and making merging of separate branches almost impossible. This should therefore not be done until 0.95 is released, to allow backporting. The main question is, should it be done at all? Max snip/ Vincent -- Vincent HennebertAnyware Technologies http://people.apache.org/~vhennebert http://www.anyware-tech.com Apache FOP Committer FOP Development/Consulting
Re: Code Quality Metrics
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Vincent,
Vincent Hennebert schrieb:
18000 PMD violations is just sick. Things like rule [1] doesn't really help
the source code. We can do that if we get a budget for
nuclear-power-plant-grade
software.
Same here I guess. Now may be the right time to launch the debate,
actually. I’ll try to gather some energy in the next days for that.
PMD contains many different check sets. The main reason for the large
number of violations is that I've enabled many of the check-sets, among
those optimizations and design, which are responsible for the large
number of error messages. Maybe we should start with the basic set and
go from there?
[1]
http://pmd.sourceforge.net/rules/optimizations.html#MethodArgumentCouldBeFinal
+1
I must say, I’ve never really grasped the benefit of doing this. I’d be
happy to be enlightened, though.
Sure:
Declaring a parameter / variable as final makes it immutable in the
method where it is used. This is:
- - required if the variable is used in an anonymous inner class (as a way
of passing parameters into one)
- - good coding practice on all methods: If a parameter is re-assigned,
the value may not be what the programmer actually intended it to be.
Example:
void someMethod(List l)
{
if (l==null) l = new LinkedList();
l.add(test);
}
or even worse, this attempt to fix it:
List someMethod(List l)
{
if (l==null) l = new LinkedList();
l.add(test);
return l;
}
Here we have a very subtle code bug, which is triggered only in a few cases:
List l1;
List l2;
...
l1 = someMethod(l1); // does not trigger
l1 = someMethod(l2); // does trigger
This rule is listed under optimizations, because final can also be
used as a hint for hotspot, and many people use it mostly for
optimization (although the performance advantage is debatable).
Max
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIUlXI+Gr+4pk71JwRAsf+AJsEKJgPlO528ArRQ2/QO05Xn0MBvwCfeRAY
VP8QJHJoHsccbd3b40xS7pU=
=/b+s
-END PGP SIGNATURE-
Re: Code Quality Metrics
- Oorspronkelijk bericht - Van: Max Berger [mailto:[EMAIL PROTECTED] Verzonden: vrijdag, juni 13, 2008 01:11 PM +1 I must say, I’ve never really grasped the benefit of doing this. I’d be happy to be enlightened, though. Sure: Declaring a parameter / variable as final makes it immutable in the method where it is used. This is: Come to think of it, I've always found it /very/ tricky to use the word 'immutable' in case of objects, especially collections. Declaring a method parameter or a member as final prevents only reassignment, but the assigned instance itself definitely remains mutable, unless additional precautions are taken to prevent structural modification after the assignment. Doing this for each and every method seems to be a bit over the top. Especially if the method body is relatively short. Anyway, I don't think there are any devs in the team at the moment who are in the habit of reassigning parameters (the fact that Vincent did not immediately see the benefit of that rule is a nice demonstration). I look at it this way: it's much worse practice to reassign parameters (especially in public methods) than it is not to declare the parameters final. In that sense, I think the behavior in a language like PL/SQL (with roots in Ada) is much better defined: disallow any parameters from being reassigned, unless the programmer explicitly declares the parameter as such (OUT or IN OUT). Maybe an idea for Java 8. ;-) Cheers Andreas
Re: Code Quality Metrics
Hi,
Thanks for your explanation.
Max Berger wrote:
[1]
http://pmd.sourceforge.net/rules/optimizations.html#MethodArgumentCouldBeFinal
+1
I must say, I’ve never really grasped the benefit of doing this. I’d be
happy to be enlightened, though.
Sure:
Declaring a parameter / variable as final makes it immutable in the
method where it is used. This is:
- required if the variable is used in an anonymous inner class (as a way
of passing parameters into one)
Agreed on this one. I’ve used that once already. (More precisely,
Eclipse told me that it would refuse to compile my file as long as
I wouldn’t declare my parameters final, and I wasn’t in the mood for
fight back then.)
- good coding practice on all methods: If a parameter is re-assigned,
the value may not be what the programmer actually intended it to be.
Hmmm. But this safeguard becomes helpful only when the method starts to
get really long, so long that you can lose track of the places where you
re-assign the local variables. So long that it should actually be split
into several sub-methods, whose small sizes make this precaution
superfluous again. That’s why I’m not convinced by the utility of this
rule.
FWIW, I do re-assign local variables sometimes (rarely). When the new
value serves the same purpose, and creating a new variable with another
name would actually look more confusing. Do I have to slash my hand for
that?
Example:
void someMethod(List l)
{
if (l==null) l = new LinkedList();
l.add(test);
}
or even worse, this attempt to fix it:
List someMethod(List l)
{
if (l==null) l = new LinkedList();
l.add(test);
return l;
}
Here we have a very subtle code bug, which is triggered only in a few cases:
List l1;
List l2;
...
l1 = someMethod(l1); // does not trigger
l1 = someMethod(l2); // does trigger
Ahem, err, what’s the bug? :-\ In both cases l1 receives a new list
containing the string “test”. In both, l1 ends up being != the parameter
of someMethod.
This rule is listed under optimizations, because final can also be
used as a hint for hotspot, and many people use it mostly for
optimization (although the performance advantage is debatable).
Moreover, the time where our last resort to further improve performance
is to declare final all the arguments of all the methods of the codebase
is no near to be reached IMO ;-)
Vincent
--
Vincent HennebertAnyware Technologies
http://people.apache.org/~vhennebert http://www.anyware-tech.com
Apache FOP Committer FOP Development/Consulting
Re: Code Quality Metrics
- Oorspronkelijk bericht - Van: Vincent Hennebert [mailto:[EMAIL PROTECTED] Verzonden: vrijdag, juni 13, 2008 08:31 PM snip / FWIW, I do re-assign local variables sometimes (rarely). When the new value serves the same purpose, and creating a new variable with another name would actually look more confusing. Do I have to slash my hand for that? I wouldn't go /that/ far. With only one hand you wouldn't be able to code anymore... and that would be quite a loss for the FOP-community. ;-) No really, the fact that you do it /rarely/ says enough. As I hinted, IMO it should actually be the other way around, i.e. reserve a keyword to mark a parameter as an 'out' parameter that is supposed to be modified/assigned by the method. It's only reassignment that is prevented, and declaring the parameter final still does not prevent you from modifying the parameter itself. As long as it remains the same instance, you will get no compiler warning. It can still come out of the method as a 'different' object. Luckily, or we would not be able to append to StringBuffer-parameters or add to List-parameters. Cheers Andreas
Re: Code Quality Metrics
Max Berger: since this came up, here is a list of tools I use for software quality checking (and all them them can check for generic list types). All of them have Eclipse and maven plugins (and ant tasks, and ) Incidentally, I would be happy too if we had some written-down guidelines (or automatic checking) for code quality in Batik. Also, I’m looking forward to the results of the retroweaver experiments, as I would like to be able to use Java 5 syntactic features in Batik. -- Cameron McCormack ≝ http://mcc.id.au/
Re: Code Quality Metrics
Dear Fop-Devs, Jeremias is right - you actually need to use the output of these reports. At this time there are: 1849 checkstyle violations 18702 pmd violations possible (find)bugs. Many of these could be automatically solved using the eclipse cleanup tools (which can actually be called on the whole src dir!). However, that would result in a change in almost every file, and making merging of separate branches almost impossible. This should therefore not be done until 0.95 is released, to allow backporting. The main question is, should it be done at all? Max Am 11.06.2008 um 09:13 schrieb Jeremias Maerki: I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. On 10.06.2008 10:01:03 Max Berger wrote: Dear Fop-Devs, since this came up, here is a list of tools I use for software quality checking (and all them them can check for generic list types). All of them have Eclipse and maven plugins (and ant tasks, and ) Checkstyle: checkstyle.sf.net (already configured in fop, so nothing needs to be done) Findbugs: findbugs.sf.net (very good - all its advices should be followed) PMD: pmd.sf.net (contains almost too many rules, some of them are debatable) I'd be willing to set up reporting for these 3 tools, so that you can check what they suggest. I usually try to follow of these rules when creating new files. Max Jeremias Maerki
Re: Code Quality Metrics
I'm scared by the thought of having a program clean our source code changing lots of files. I prefer the approach that the devs shall try to improve the code while they are working on it. BTW, I think there's one or two rules in our Checkstyle file that probably should/could be removed. For example, the frequent warning about field hiding and Utility classes should not have a public constructor. But that's a separate discussion I guess. 18000 PMD violations is just sick. Things like rule [1] doesn't really help the source code. We can do that if we get a budget for nuclear-power-plant-grade software. [1] http://pmd.sourceforge.net/rules/optimizations.html#MethodArgumentCouldBeFinal On 12.06.2008 08:28:35 Max Berger wrote: Dear Fop-Devs, Jeremias is right - you actually need to use the output of these reports. At this time there are: 1849 checkstyle violations 18702 pmd violations possible (find)bugs. Many of these could be automatically solved using the eclipse cleanup tools (which can actually be called on the whole src dir!). However, that would result in a change in almost every file, and making merging of separate branches almost impossible. This should therefore not be done until 0.95 is released, to allow backporting. The main question is, should it be done at all? Max Am 11.06.2008 um 09:13 schrieb Jeremias Maerki: I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. On 10.06.2008 10:01:03 Max Berger wrote: Dear Fop-Devs, since this came up, here is a list of tools I use for software quality checking (and all them them can check for generic list types). All of them have Eclipse and maven plugins (and ant tasks, and ) Checkstyle: checkstyle.sf.net (already configured in fop, so nothing needs to be done) Findbugs: findbugs.sf.net (very good - all its advices should be followed) PMD: pmd.sf.net (contains almost too many rules, some of them are debatable) I'd be willing to set up reporting for these 3 tools, so that you can check what they suggest. I usually try to follow of these rules when creating new files. Max Jeremias Maerki Jeremias Maerki
Re: Code Quality Metrics
I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. On 10.06.2008 10:01:03 Max Berger wrote: Dear Fop-Devs, since this came up, here is a list of tools I use for software quality checking (and all them them can check for generic list types). All of them have Eclipse and maven plugins (and ant tasks, and ) Checkstyle: checkstyle.sf.net (already configured in fop, so nothing needs to be done) Findbugs: findbugs.sf.net (very good - all its advices should be followed) PMD: pmd.sf.net (contains almost too many rules, some of them are debatable) I'd be willing to set up reporting for these 3 tools, so that you can check what they suggest. I usually try to follow of these rules when creating new files. Max Jeremias Maerki
Re: Code Quality Metrics
Jeremias Maerki wrote: I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. As Karen has been inactive for some time now, I can only assume that one of the developers wants a sex change. Who is it? -- Peter B. West http://cv.pbw.id.au/ Folio http://defoe.sourceforge.net/folio/
Re: Code Quality Metrics
;-) I don't make assumptions what sex the people are that work with FOP (committer or not). In Switzerland, it's best practice to include both forms if you address a group of people with unknown composition. On 11.06.2008 09:44:39 Peter B. West wrote: Jeremias Maerki wrote: I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. As Karen has been inactive for some time now, I can only assume that one of the developers wants a sex change. Who is it? -- Peter B. West http://cv.pbw.id.au/ Folio http://defoe.sourceforge.net/folio/ Jeremias Maerki
Re: Code Quality Metrics
Well, Glen is the only FOP developer who's actually seen me in person... Although Jeremias has seen my avatar... ;-) Clay On 6/11/08, Jeremias Maerki [EMAIL PROTECTED] wrote: ;-) I don't make assumptions what sex the people are that work with FOP (committer or not). In Switzerland, it's best practice to include both forms if you address a group of people with unknown composition. On 11.06.2008 09:44:39 Peter B. West wrote: Jeremias Maerki wrote: I'm using FindBugs (as Eclipse plug-in) for some time now and it is really good. Not that I can really say yes to 100% of the suggestions. But about 98%. I'm not sure about the benefit of those reports. We've had the Checkstyle report for years now, but I doubt many people look at that often. Having those tools as IDE plug-ins is much more useful. But that needs to be set up by every dev him/herself. As Karen has been inactive for some time now, I can only assume that one of the developers wants a sex change. Who is it? -- Peter B. West http://cv.pbw.id.au/ Folio http://defoe.sourceforge.net/folio/ Jeremias Maerki -- Sent from Gmail for mobile | mobile.google.com Regards, The Web Maestro -- [EMAIL PROTECTED] - http://ourlil.com/ My religion is simple. My religion is kindness. - HH The 14th Dalai Lama of Tibet
Re: Code Quality Metrics
Hi Max, By all means, go for it. That can’t hurt, IMO, and this will probably be quite helpful (or scary, maybe ;-) ). Of course, just setting them up will not be enough. Their suggestions will also have to be followed. Given that there are already something like 18,000 checkstyle warnings in the current codebase... Still. Thanks, Vincent Max Berger wrote: Dear Fop-Devs, since this came up, here is a list of tools I use for software quality checking (and all them them can check for generic list types). All of them have Eclipse and maven plugins (and ant tasks, and ) Checkstyle: checkstyle.sf.net (already configured in fop, so nothing needs to be done) Findbugs: findbugs.sf.net (very good - all its advices should be followed) PMD: pmd.sf.net (contains almost too many rules, some of them are debatable) I'd be willing to set up reporting for these 3 tools, so that you can check what they suggest. I usually try to follow of these rules when creating new files. Max -- Vincent HennebertAnyware Technologies http://people.apache.org/~vhennebert http://www.anyware-tech.com Apache FOP Committer FOP Development/Consulting
Re: Code Quality Metrics
On Jun 10, 2008, at 12:26, Vincent Hennebert wrote: By all means, go for it. I second this. That can’t hurt, IMO, and this will probably be quite helpful (or scary, maybe ;-) ). Of course, just setting them up will not be enough. Their suggestions will also have to be followed. Given that there are already something like 18,000 checkstyle warnings in the current codebase... On that note, I'm currently evaluating IntelliJ, which has some really nice Code Inspection features. It warns about redundant statements, for example: unnecessary casts, variable that are assigned but never used, while-statements that don't loop... You'd be surprised how many of these are scattered over the codebase. Not really anyone's fault, just a result of many refactoring cycles in succession. As you said, the result could turn out to be scary... :-) Cheers Andreas
Re: Code Quality Metrics
Dear Fop-Devs, for the actual implementation, I think it would be a good idea to create a second lib-directory (e.g. buildsupport, or buildlib), and add the required libs there, so that we're all using the same tools. These libs would only be needed during build, and not during deployment. Libs which are BSD (pmd, retroweaver /translator) would be added there directly, for others (checkstyle, findbugs) I'd provide a readme on where to find them. Right now this is the case with lib/, but everything in lib/ is also used during run-time, whereas I would explicity like to exclude these build-time dependencies. This would increase the size of the fop svn directory, but it would also make it easier for developers to get started. Please comment. On that note, I'm currently evaluating IntelliJ, which has some really nice Code Inspection features. It warns about redundant statements, for example: unnecessary casts, variable that are assigned but never used, while-statements that don't loop... nice. This could definitely be complimentary. As you said, the result could turn out to be scary... :-) It will be. Cheers Andreas Max
Re: Code Quality Metrics
Hi Max, Max Berger wrote: Dear Fop-Devs, for the actual implementation, I think it would be a good idea to create a second lib-directory (e.g. buildsupport, or buildlib), and add the required libs there, so that we're all using the same tools. These libs would only be needed during build, and not during deployment. Libs which are BSD (pmd, retroweaver /translator) would be added there directly, for others (checkstyle, findbugs) I'd provide a readme on where to find them. Right now this is the case with lib/, but everything in lib/ is also used during run-time, whereas I would explicity like to exclude these build-time dependencies. Actually there is already a lib/build directory that serves that purpose. You could use it, or if you prefer to have a separate directory you can move the content of lib/build into the new one. Your choice :) This would increase the size of the fop svn directory, but it would also make it easier for developers to get started. Please comment. On that note, I'm currently evaluating IntelliJ, which has some really nice Code Inspection features. It warns about redundant statements, for example: unnecessary casts, variable that are assigned but never used, while-statements that don't loop... nice. This could definitely be complimentary. As you said, the result could turn out to be scary... :-) It will be. Cheers Andreas Max Vincent -- Vincent HennebertAnyware Technologies http://people.apache.org/~vhennebert http://www.anyware-tech.com Apache FOP Committer FOP Development/Consulting
