JFactory, the Pedant asks why?

[Paladin]

Going on a code walk, and feeling a bit like the little mustachio'ed
man at the end of the parade in "Peabody's Improbable History," I feel
the need to ask a few why's of this code:

1) Why are all the methods in this abstract class declared static
except _createMailer and getStream? Is this an oversight or is there a
reason I haven't yet stumbled across?

2) Why is _createMailer not declared as private? The underscore by
convention indicates a private function, as does the PHPDoc comment
tag.

3) Why is getStream not declared as public? I realize the default is
public, but all the other public methods are declared as public.

4) Why is getStream "hidden" down below the private functions, rather
than among the public functions?

If the questions seem a bit anal-retentive, look at them from this
point of view: When someone comes to the code from the outside, these
inconsistencies (and the inconsistencies between the functions
themselves and their descriptions in PHPDoc, as when getApplication
can take 3 args but only two are listed in the PHPDoc tags, or when
getURI takes an arg, but none are defined in the PHPDoc tags, as just
two examples) cause not only a momentary disconnect as the questions
raise issues that need to be resolved before touching the code, but
even once they *are* resolved they leave a residue of distrust for
anything else in the documentation or declarations. Or is it
considered a "feature" of Joomla development that none can think of
making a single mod without committing the entire codebase to memory?

If, as I suspect, the answer to all of the above questions is that the
anomalies are caused by oversight, I can submit patches for them (and
any others I come across as I continue my code walk). That's not a
problem, and I'm not complaining about it. But if so, their existence
does raise the question of whether there should be a "nose" (for lack
of a better term) who detects smells in the code files either recently
committed or due for committing, enforcing the Joomla "house style"
and being in general a code sweeper.

[Wilco Jansen]

Hi,

I am not 100% but I think there is not a specific reason for this inconsistency. You bring up a point that I ran in to when researching the possibilities for getting an continuous integrated environment up and running for Joomla (see http://jfoobar.org/blog/303-continuous-integration-in-php-a-closer-look-at-coding-standards-.html for the full article about my findings). If you run a code sniffer (phpcs) and use the coding standards of Joomla (which derived from the PEAR standards), you will find a ton of inconsistencies in the current codebase, this includes class/method definitions and also usage of naming conventions.

In the ideal situation you would have close to 100% code coverage with the unit tests, so alteration of the code will not lead to breaking application logic. There is an initiative going on to get the framework fully tested, this is not the case (yet). So creating a patch would certainly be an idea, testing if it does not break anything will be a tedious task.

I would suggest an initiative to make sure the codebase complies with the basics of the coding standards, a lot of the problems you mention will then simply be resolved, and if possible in the 1.6 codebase so you know you have a clean start for future versions.

Just my 2 cents.

[Hannes Papenberg]

Hi,
most of these errors are due to oversight. I would be happy if you would
provide patches for these issues and if you have more issues, compile a
list and call me on skype and we can go through them and discuss them.

Hannes

Paladin schrieb:

[Louis Landry]

Great initiative!  A lot of these inconsistencies are due to oversight, some are due to some of us being a little sloppy in our excitement as we get things done in one place or another, yet more of them are probably just mistakes.  Glad to have your help in normalizing those sorts of issues.  

I think when you run across issues you see or have questions about it would be good to discuss them on list so that everyone has the benefit of the discussion as well as the benefit of having the history documented.

Cheers,

Louis

--
Development Coordinator
Joomla! ... because open source matters.
http://www.joomla.org

[Paladin]

On Oct 19, 10:32 am, Hannes Papenberg <hackwa...@googlemail.com>
wrote:

> Hi,
> most of these errors are due to oversight. I would be happy if you would
> provide patches for these issues and if you have more issues, compile a
> list and call me on skype and we can go through them and discuss them.

OK, I'll probably have a complete set of JFactory patches for you
later this week. I'm working on unit testing, which is why questions
like these are coming up.

I've been addressing only the inconsistencies that were obvious to me,
but now I'll also take a look at Wilco's blog post and see if I can
address some of those issues as well. I don't insist on one standard
over another; I *do* insist on a standard. ;{>}

I started out thirty years ago fixing and extending other people's
code, and even today it forms a large part of what I do. I've had devs
get offended when I came along and twitched the little things like
this. I understand deadline pressure and frustration, but I also know
too well the issues of coming along and trying to fix code you had no
part in writing. It's all about the details.

"Code as if whoever maintains your code is a violent psychopath who
knows where you live."

[Andrew Eddie]

Thanks Paladin

Appreciate the effort you are putting in.

Regards,
Andrew Eddie
http://www.theartofjoomla.com - the art of becoming a Joomla developer




2009/10/21 Paladin <arlen....@gmail.com>:

[Paladin]

On Oct 19, 10:59 am, Louis Landry <louis.lan...@joomla.org> wrote:
> Great initiative!  A lot of these inconsistencies are due to oversight, some
> are due to some of us being a little sloppy in our excitement as we get
> things done in one place or another, yet more of them are probably just
> mistakes.  Glad to have your help in normalizing those sorts of issues.
> I think when you run across issues you see or have questions about it would
> be good to discuss them on list so that everyone has the benefit of the
> discussion as well as the benefit of having the history documented.

OK, next simple question:

getFeedParser returns parsed object or false on failure

getXMLParser returns parsed object or null on failure

The difference is subtle (null is false, but false is not null) and
maybe of no significance, but for the sake of simplicity, shouldn't we
pick one and go with it for all cases?

I'm thinking both should return null on failure, that way we don't
break anything looking for false. Objections?

[Louis Landry]

Sounds good to me.  Good catch.

[Paladin]

On Oct 19, 10:29 am, Wilco Jansen <jansen.wi...@gmail.com> wrote:
> You bring up a point that I ran in to when researching the
> possibilities for getting an continuous integrated environment up and

> running for Joomla (seehttp://jfoobar.org/blog/303-continuous-integration-in-php-a-closer-lo...

> the full article about my findings). If you run a code sniffer (phpcs)
> and use the coding standards of Joomla (which derived from the PEAR
> standards), you will find a ton of inconsistencies in the current codebase,
> this includes class/method definitions and also usage of naming conventions.

I can try and put together a custom coding standard for the php code
sniffer (phpcs) if there's any interest in using it. From what I've
been able to determine so far, the joomla divergences from PEAR
standards are:

1) No defined max line length (PEAR is 85 chars)
2) Required Doc Blocks differ between file and class docblock (PEAR is
the same docblock for each)
Joomla File block: version, package, copyright, and license, in
that order, all required.
Joomla Class block: package, subpackage, link, see, since,
deprecated, in that order, (only "package" required to be there?)
3) Slight difference in version tag (PEAR requires CVS: or SVN: at
start)
4) Slight difference in syntax of copyright tag (PEAR is simply year
and copyright holder)
5) Switch/case indents the cases one level (PEAR does not)
6) If statements. Hard to determine the Joomla standard, as the wiki
contradicts both itself and PEAR.

Contradiction:
wiki (<a href="http://docs.joomla.org/
Coding_style_and_standards">http://docs.joomla.org/
Coding_style_and_standards</a>) in the Control Structures diagram
shows first this style for else:

} else
{

and then this:

}
else {

while PEAR is:

} else {

I have no idea what should be right (my personal pref is the PEAR
way). Comments? Any other coding bugaboos you want this to rule out?

I can limit the allowed package names if you like, but when I start
getting to specific (for example, forcing the copyright statement to
include 2005-2009 for years) i'd need to go back and tweak the rules
more often, and I'd rather keep the rules as general as possible.

Note: I'm tinkering with one for my own use right now. This question
is more about having an "official" one contributors can use to check
their files against, or which might be a pre-commit hook on subversion
to enforce Joomla coding standards as a step toward continuous
integration. Or, if you'd rather, I can toss my version into my Github
area as an open source project that everyone can contribute to and
pull from as new rules are defined.

[Niels Braczek]

Paladin schrieb:

> I can try and put together a custom coding standard for the php code
> sniffer (phpcs) if there's any interest in using it.

I'm very interested in that!

> 6) If statements. Hard to determine the Joomla standard, as the wiki
> contradicts both itself and PEAR.

> I have no idea what should be right (my personal pref is the PEAR
> way). Comments? Any other coding bugaboos you want this to rule out?

I definitly prefer the PEAR style, too.

Regards,
Niels

--
| http://www.kolleg.de · Das Portal der Kollegs in Deutschland |
| http://www.bsds.de · BSDS Braczek Software- und DatenSysteme |
| Webdesign · Webhosting · e-Commerce · Joomla! Content Management |
------------------------------------------------------------------

[Paladin]

On Oct 21, 4:28 pm, Niels Braczek <nbrac...@bsds.de> wrote:
> Paladin schrieb:
>
> > I can try and put together a custom coding standard for the php code
> > sniffer (phpcs) if there's any interest in using it.
>
> I'm very interested in that!

OK, since I think about things like this better when I have something
concrete in front of me, behold http://github.com/Paladin/Joomla-Code-Sniffer
and tremble with fear! (or laughter.)

It's not complete, but I think I captured most things. Take it out for
a walk and tell me if it digs up your flower beds, wets your pant leg
or commits some other faux pas.

The flags it raises in the stock factory.php number 84 of which about
80% are in the doc blocks.

While I didn't insert credits to me in all files I've changed I
maintained the original names of the authors of the PEAR sniffer that
I derived this from, and kept the license as BSD. (I know, Joomla
loves the GPL, but since I don't care to start a licensing war, let's
just say that since I derived this from BSD licensed code, I thought I
should keep it that way.)

[Paladin]

OK, the patches are in the mail. The most radical changes to it were
in the getFeedParser method, specifically, this block of changed code,
which I'll explain the reasoning behind for public comment, as Louis
suggested:

$simplepie = new SimplePie($url, JPATH_CACHE, $cache_time);
if ($simplepie->data) {
$data = $simplepie;
} else {
JError::raiseWarning('SOME_ERROR_CODE', JText::_('ERROR LOADING
FEED DATA'));
$data = null;
}
return $data;

Instead of $data = $simplepie, the original code simply returned
$simplepie at this point. Personally, I don't have anything against
early returns, in fact using them can greatly improve code simplicity
and readability, but in this case the gain wasn't that great, and the
style of this method was completely inconsistent with getXMLParser.
One of the two functions should be changed, and since I was already
changing the error return in this method from false to null, I chose
to keep the changes located in this method, and change it to mimic the
style of getXMLParser, namely collect the return values in a variable
and return it at the end.

Had the else block been more complicated in this method, I'd have kept
the early return and FWIW I still wonder if this approach would have
been cleaner:

$simplepie = new SimplePie($url, JPATH_CACHE, $cache_time);
if ($simplepie->data) {
return $simplepie;
}
JError::raiseWarning('SOME_ERROR_CODE', JText::_('ERROR LOADING FEED
DATA'));
return null;

Everything else in the patch file is unremarkable, more garbage
collection than anything else.

I also submitted a second patch file addressing my personal beef with
jimport (it's a waste of electrons) but implementing that patch is
best left to a team decision over retaining jimport (which is why I
put it into a second patch file, all by itself). I don't believe in
changes for the sake of changes, so I would recommend against
implementing the second patch unless the team commits to dropping
jimport. (And if it doesn't I'll include replacing JLoader::import
references with jimport when I run across them in the future. Using
them both interchangeably only increases the confusion factor in the
code; I'd prefer to drop jimport, but whichever one is blessed should
be the one the code is committed to using.)