Ernst,This looks great. Thanks for providing the patch against trunk as well, when we're fixing bugs we try to fix them in trunk first then merge the changes back into any branches that are also affected.
I agree with Chris that the bite-sized patches are the way to go, it helps keep the change size down and reduces the risk of one large commit causing unintended side-effects.
I've tested and committed the patch in trunk (46039), 3.1-patches (46040) and 3.0-patches (46042).
If you're interested in continuing to address these issues I'd be happy to help review a few patches and I'd imagine we could propose you for commit access in fairly short order so that you could resolve these yourself directly. Also if you're unsure how to address an issue feel free to ask about specific bits of code here and developers will do there best to answer.
As for this being part of the uPortal CI build ... it already is! Visit http://developer.jasig.org/ and look at the Nightly Build Sites for uPortal. If you go to the uPortal Source module then Project Reports we have both FindBugs and PMD reports being built and published nightly.
Thanks again! -Eric Ernst-Jan Verhoeven wrote:
Hi all,I'm a developer for the University of Amsterdam and we are planning to run uPortal 3.1 in production in a few months. Of course we like the system to be as stable as possible, and as me and my colleagues are huge fans of automated code-review tools we ran the automated (byte-)code review tool 'Findbugs' on uPortal.Not all issue-categories reported by Findbugs are equally easy or rewarding to fix. I picked the '(possible) null pointer derefences' category as it usually contains issues that are both easy and rewarding to fix and spent an afternoon trying to fix these issues. I have included my efforts in a Jira report:http://www.ja-sig.org/issues/browse/UP-2443This was more of an exercise to get a feeling of the maintainability of the code, but we intend to do more fixes like this.Other nice candidates seem e.g.'method may fail to close database resource' (33 reported Findbugs-issues) 'Method may fail to clean up stream or resource' (40 reported Findbugs-issues)although in my experience (and/or opinion) Findbugs usually reports a very low number of false positives; so it may not be that useful to 'zoom in' on specific categories.How do others feel about this 'bottom-up' way of fixing possible issues in the uPortal code-base?-Ernst-Jan Verhoeven
smime.p7s
Description: S/MIME Cryptographic Signature
