On Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrett<agarrett(a)wikimedia.org> wrote:
I honestly don't think this is that important. We
should err on the
side of public, not private or protected.
Very frequently, I've had to change visibility on functions because
somebody hasn't anticipated a case where a class function might be
usefully called from outside that class.
On the other hand, I've often wanted to refactor code but been unable
to because the functions were public or protected and I didn't want to
break any third-party extensions that might call them. If a function
is private and could usefully be public, then just make it public, no
problem.
The only people who are harmed by too many private functions are
people without commit access, who can't just make the functions
public. Since these are also the only people whose code could be
broken by changing a public function, though -- if the code is
committed, then the one who changes the public function can just
change the caller -- you can make a good case that it's better to make
everything public, and let extension authors rely on the public
functions if they want without any guarantee that the functions will
remain stable. They'd probably prefer that to having the function
private and not being able to even try to use it.
Generally speaking, I always make my functions private if I don't
explicitly intend to make them part of the class interface for
third-party use. But I can see the other side too, so I don't know if
a firm policy is the best course of action.
On Wed, Aug 12, 2009 at 9:24 AM, Victor Vasiliev<vasilvv(a)gmail.com> wrote:
What comment would you except to "public function
getArticleText()"?
Well, I can't write a very good comment since I've never read that
function or even any significant part of that class until now. (I'm
looking at RawPage::getArticleText() here.) But maybe something like:
/**
* Returns the parsed HTML corresponding to this page for action=raw output,
* and outputs some HTTP headers.
*
* @return string Parsed HTML suitable for echoing
*/
private function getArticleText() {
The documentation is cursory because it's really just a helper method
that's designed to be called in exactly one place, so anyone who wants
to use it is already making changes to RawPage and will as likely as
not want to read and/or change the source of the method.
If you don't want to make it private for some reason, then I'd add
@private to the documentation and put a note saying what method you
actually want to call. It has various side effects and
RawPage-specific logic that make it very unlikely to be useful to any
other classes.
If you were referring to a hypothetical public method in, say,
Revision, then I'd expect the comment to include all information that
callers might like to know, such as:
* Is the text parsed or unparsed? If parsed, what method should be
used to get unparsed text? If unparsed, what method to get parsed
text?
* What's the performance impact like? Does this do any batching or
caching? Does every call result in a database query? Does it call
the parser? Does it use memcached? Is it possible that this will
return outdated text, or will it definitely be up-to-date?
* Is any kind of permissions checking done? Will this retrieve text
for revisions that are deleted, oversighted, rev_deleted, or otherwise
hidden? Does it check whether the current user has read permission?
* What happens if there's an error, like the revision doesn't exist?
Is an exception thrown, or some magic value returned? What sorts of
errors should we expect to see? Can we assume there are no errors?
E.g., a low-level function that always returns the text even if
deleted, and where the revision object was created with a constructor
that verified that the revision was legit, might never return an error
unless the database couldn't be reached or was manually tampered with
(in which case the request should probably just die anyway).
The answers don't have to be lengthy, and they can refer to
documentation of other methods within reason. But they *should* be
there, in a form where most MediaWiki developers can understand them.
I think all those questions could be answered in about three or four
lines, say, depending on what the answers are. I really have to go
now, or else I'd commit a good comment for Revision::get(Raw)Text() to
show you what I mean.
In fact, Revision::getText() already has 11 lines of comments.
Revision::getRawText() has only 2. I think both could be improved a
bit.
By the way, is there a policy which describes where
should we use
get*/set* methods and where should we use public fields?
I always use private member variables. Some other developers always
use public member variables. It's pretty much the same story as with
public/private methods.