Re: [EGIT PATCH] Comment private modifier to improve performace.

From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Feb 3, 2008, 22:25 UTC
Message-ID: <200802032325.47844.robin.rosenberg@dewire.com>
In-Reply-To: <47A61A30.3000904@intelinet.com.br>

söndagen den 3 februari 2008 skrev Roger C. Soares:

> Ok, so some more points for your consideration:
> 
> . I saw this /* private */ notation on eclipse code and found it 
> interesting.

It is.

Show 5 quoted lines

> . I won't bother measuaring any single case to make sure it is not 
> impacting performance under some circunstance, so resolving those 
> warnings puts me in the safe area. On the other hand, I think it is a 
> lot easier to tell if a patch is breaking encapsulation in a bad way 
> just by reviewing it, which is something that is already done.

We do not have a large number of people reviewing our code at this stage, so I would not trust that at the moment.

> Especially if it has the private modifier commented out. Someone can 
> even do a script to uncomment them and verify that it still builds 
> without errors.

There is more to reviewing than just looking at the diffs.

Show 6 quoted lines

> . The ui part isn't supposed to be reused by other projects, so I think 
> encapsulation there is less important than for jgit. But even so, the 
> default modifier (or package-private) is good enough for encapsulation. 
> Other projects shouldn't write code in the the same packages from jgit, 
> if they do so they know that they are using internal things and they can 
> run into problems in the future.

I'm thinking more about internal encapsulation between the classes within the project. Many of the classes in the ui (and jgit) packages have little in common other than that they contribute to the same overall goal (an Eclipse plugin). I guess that is what happens when things get build a small patch at a time. Obviously we could have more packages, but that might make things worse by forcing us to have more public methods and having a lot of one and two class packages feels wrong too (though it actually may be right). I don't think I'll look at that right now though.

-- robin