Would like to Echo Ben's feedback - thanks! Andrea, that is very good. Will this document be moved to the developer > guide? >
Agree we should sort things out in the wiki and then move contents to developers guide. Other things I like to watch for: > > - Code style/formatting, no tabs, use of modern Java idioms including > generics, foreach, and try-with-resources (which should reduce use of > finally). I prefer JUnit 4 over JUnit 3. > When moving to the developers guide I would like to link to any style guide, rather than duplicate. - Resource leaks: any time a close method is used outside a finally or > try-with-resources block it attracts my attention. > I do think we can update the developers guide with this kind of information, even if it is just "these things are probably wrong and will slow down your code review". - Malicious or dangerous code from untrusted submitters. > > - static instances / singletons are a great way of introducing concurrency > issues; you have this in your "Thread safety" section already but it might > be worth mentioning that static instances are a red flag. (gt-app-schema > DataAccessRegistry I am looking at you.) > > Do you use or recommend any linting tools that support differential > operation, that is, reporting new suspect code practices introduced by a > commit? Do you think such tools are helpful for this project? > I use FindBugz when reviewing. I also think that we need a pull request submitter guide. Reviews are less > work when they are prepared with review in mind. Should pull request > submitter guidelines be part of the same document? Ideally I would like this to be the same document to avoid having the two drift and be clear about expectations.
------------------------------------------------------------------------------
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
