[Geoserver-devel] Opinion on a pull request

GeoServer GeoServer Developer
1

Hi,
can devs share opinions on this pull request?
https://github.com/geoserver/geoserver/pull/483

Personally I prefer to write less and use package private instead of private
unless there is a performance reason to do otherwise (hotspot can inline private
methods for efficiency sake), but if everybody else feels otherwise
I guess we can start accepting this kind of pull request.

The guy will have to rewrite basically everything I’ve touched in the last two
years though, maybe more.

Cheers
Andrea

–

== Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information ==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

2

On Tue, Feb 18, 2014 at 8:22 AM, Andrea Aime
<andrea.aime@anonymised.com>wrote:

Hi,
can devs share opinions on this pull request?
https://github.com/geoserver/geoserver/pull/483

Personally I prefer to write less and use package private instead of
private
unless there is a performance reason to do otherwise (hotspot can inline
private
methods for efficiency sake), but if everybody else feels otherwise
I guess we can start accepting this kind of pull request.

I also prefer to write less unless I want to mark something explicitly

private, for instance if writing a base class to make it clear what is
available to a subclass.

While I am not against the change I generally don't see much of a point in
applying them to existing classes. If anything they make the class harder
to work with across branches (unless the changes are also applied
retroactively). As well unless the changes are applided across the board it
introduces more inconsistency.

Generally I work from the rule of thumb of respecting conventions already
used in a class, be it naming conventions, modifiers, formatting, etc....
and only try to apply this sort of stuff on new classes.

$0.02

The guy will have to rewrite basically everything I've touched in the last

two
years though, maybe more.

Cheers
Andrea

--
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information ==

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.

http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
*Justin Deoliveira*
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive <https://twitter.com/j_deolive&gt;

3

I concur with Justin that modifying existing code will mostly cause conflicts but also agree there are places it might make sense. Especially if it leads to clarity in design or prevents mistakes.

I’m not a big fan of the final method parameter convention though concede it’s saved me once or twice in the past from the self-assignment-setter mistake. This has been made less useful as most IDEs will flag this. The only other potential prevention scenario I can think of is dealing with local primitives - http://programmers.stackexchange.com/a/115711 (there are other decent points there, too)

In the past I’ve also seen performance gains from private final fields but VM enhancements may have erased this.

···

On Tue, Feb 18, 2014 at 9:15 AM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–

Ian Schneider
Software Engineer | Boundless
ischneider@anonymised.com

On Tue, Feb 18, 2014 at 8:22 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
can devs share opinions on this pull request?
https://github.com/geoserver/geoserver/pull/483

Personally I prefer to write less and use package private instead of private
unless there is a performance reason to do otherwise (hotspot can inline private
methods for efficiency sake), but if everybody else feels otherwise
I guess we can start accepting this kind of pull request.

I also prefer to write less unless I want to mark something explicitly private, for instance if writing a base class to make it clear what is available to a subclass.

While I am not against the change I generally don’t see much of a point in applying them to existing classes. If anything they make the class harder to work with across branches (unless the changes are also applied retroactively). As well unless the changes are applided across the board it introduces more inconsistency.

Generally I work from the rule of thumb of respecting conventions already used in a class, be it naming conventions, modifiers, formatting, etc… and only try to apply this sort of stuff on new classes.

$0.02

The guy will have to rewrite basically everything I’ve touched in the last two
years though, maybe more.

Cheers
Andrea

–

== Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information ==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–

Justin Deoliveira
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive

4

On Tue, Feb 18, 2014 at 5:15 PM, Justin Deoliveira <
jdeolive@anonymised.com> wrote:

While I am not against the change I generally don't see much of a point in
applying them to existing classes. If anything they make the class harder
to work with across branches (unless the changes are also applied
retroactively). As well unless the changes are applided across the board it
introduces more inconsistency.

I guess we could cherry-pick them on previous series as well, or ask that
the pull requests be done on master and stable.

While on one side I don't like style changes, on the other one, I hate to
discourage a new contributor more.

So, what if we merge them, provided the changes are uniformly applied to
all working branches?

Looking at a possible objection about "horizontal" consistency, that is,
among classes in the same branch,
well, we don't have it now anyways, for example, I know Simone uses final
and private any time he can.

Cheers
Andrea

--
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information ==

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

5

I tend to like to use final - as much for developers reading the code as the compiler. As an example I caught my self wanting to make the IOUtils utility class final to describe use (even though it had a private constructor which makes it final for developers).

I think we should be more sensitive to our internal API as we open the gate with WPS processes, controlling visibility is good step - especially as Konstantin has done the “grunt” work.

···

On Tue, Feb 18, 2014 at 5:15 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

I guess we could cherry-pick them on previous series as well, or ask that the pull requests be done on master and stable.

While on one side I don’t like style changes, on the other one, I hate to discourage a new contributor more.

So, what if we merge them, provided the changes are uniformly applied to all working branches?

Looking at a possible objection about “horizontal” consistency, that is, among classes in the same branch,
well, we don’t have it now anyways, for example, I know Simone uses final and private any time he can.

Cheers
Andrea

–

== Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information ==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

While I am not against the change I generally don’t see much of a point in applying them to existing classes. If anything they make the class harder to work with across branches (unless the changes are also applied retroactively). As well unless the changes are applided across the board it introduces more inconsistency.