On Wed, Aug 12, 2009 at 8:31 AM, Andrew Garrettagarrett@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 Vasilievvasilvv@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.