git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Robin Rosenberg <robin.rosenberg@dewire.com>
To: "Roger C. Soares" <rogersoares@intelinet.com.br>
Cc: git@vger.kernel.org
Subject: Re: [EGIT PATCH] Comment private modifier to improve performace.
Date: Sun, 3 Feb 2008 23:25:47 +0100	[thread overview]
Message-ID: <200802032325.47844.robin.rosenberg@dewire.com> (raw)
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.

> . 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.

> . 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

      reply	other threads:[~2008-02-03 22:26 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-02  2:23 [EGIT PATCH] Comment private modifier to improve performace Roger C. Soares
2008-02-03  1:01 ` Robin Rosenberg
2008-02-03  2:26   ` Robin Rosenberg
2008-02-03 20:03     ` Roger C. Soares
2008-02-03 22:14       ` Robin Rosenberg
2008-02-03 19:46   ` Roger C. Soares
2008-02-03 22:25     ` Robin Rosenberg [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200802032325.47844.robin.rosenberg@dewire.com \
    --to=robin.rosenberg@dewire.com \
    --cc=git@vger.kernel.org \
    --cc=rogersoares@intelinet.com.br \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).