Coding standards request

[Rafael Corral]

Hopefully this is the right place for this.
This has finally annoyed me enough to post about it.

Throughout the Joomla Framework and CMS code, the alignment of
assignments is done by using tabs until all of the equal signs are
aligned, like this:
$app = JFactory::getApplication();
$lang = JFactory::getLanguage();
$model = $this->getModel();
$table = $model->getTable();
$data = JRequest::getVar('jform', array(), 'post', 'array');
$checkin = property_exists($table, 'checked_out');
$context = "$this->option.edit.$this->context";
$task = $this->getTask();

This looks fine assuming everyone has their editor set to 4 spaces = 1
tab.

The correct way to do this is to use spaces to indent these
assignments like this:
$app = JFactory::getApplication();
$lang = JFactory::getLanguage();
$model = $this->getModel();
$table = $model->getTable();
$data = JRequest::getVar('jform', array(), 'post', 'array');
$checkin = property_exists($table, 'checked_out');
$context = "$this->option.edit.$this->context";
$task = $this->getTask();

It may not look correctly after I send this message on google groups,
so I am attaching two screenshots.
https://img.skitch.com/20111006-xc3x9csgx2g6w879u8g74qxwmq.jpg
https://img.skitch.com/20111006-nd9rcqs18psukxd9kfaju8axpa.jpg

Could we add this to the coding standards?

[Gary Glass]

That brings to mind a question I was wondering about. Is there a formal code
review system for the framework? Joomla's code is getting better, but there
are a number of things like this that could be caught in code reviews.

______________________________
Gary Glass
http://ShutterGlass.com
http://OnlyBegotten.com

--
You received this message because you are subscribed to the Google Groups
"Joomla! Framework Development" group.
To post to this group, send an email to
joomla-dev...@googlegroups.com.
To unsubscribe from this group, send email to
joomla-dev-frame...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/joomla-dev-framework?hl=en-GB.

[Michael Babker]

The platform has a code sniffer which is based on the PEAR standard and
modified a bit. Instructions to set up the platform's sniffer are on the
wiki on GitHub and in the documentation.

Michael Babker
Owner, FLBab.com

³You can¹t connect the dots looking forward; you can only connect them
looking backwards. So you have to trust that the dots will somehow connect
in your future. You have to trust in something ‹ your gut, destiny, life,
karma, whatever. This approach has never let me down, and it has made all
the difference in my life.² - Steve Jobs

[Gary Glass]

That's good, but it's not code review. Maybe the community just isn't interested in code review, which is fine, I'm just interested in improving the product.

[Phill Brown]

Personally I prefer no tabbing or extra spacing between the assignment
of variables.

Regards,

Phill Brown
M  04 2481 9754
Bathurst Software Solutions
-------------------------------------------------------------------------------------------------------------------

[Andrew Eddie]

@Rafael. We did a sweep to make the formatting consistent a while
back and to make that an efficient process, we abandoned lining up the
"="'s. Some people like it that way, others don't, but you are right
in saying if we do that we have to use spaces. We are open to that
being an option providing there is some guidance on when to align
them, and when not to, and someone does the work of doing it :) (don't
look at me, I've done my formatting penance once and that was enough,
hehe).

However my preference is definitely not to worry about aligning them
anymore (I'm with you Phill). We have sniffs in place to scan
docblocks but for more than that I can't think of an easy, intuitive
way to approach it that doesn't tie us in knots or personal
preference.

@Gary, there are a couple of things we rely on. One is automated
testing and that, hopefully, covers most of the bases in terms of "is
the API doing what it is supposed to". Of course that can only go so
far so each pull request is eye-balled by one or more of the platform
maintainers and, hopefully, other people interested in the changes.
Unfortunately that still doesn't mean there won't be problems because
it's hard to tell if regression is introduced downstream, such as in
the Joomla CMS, not to mention that sometimes you only find odd
problems when you use something in practice. Ideally we need to get
the CMS project set up so that it is pulling a copy of the framework,
probably nightly, and running its suite of system tests. That should
be able to report back any obvious glitches sooner rather than later.

Does that answer the question or is there something else you had in mind?

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla 1.7 developers

[Gary Glass]

Definitely something else. Good professional-quality code reviews include things like:

1. Do all non-private members have phpdoc comments?
2. Is the commenting correct? Does it generate correct PHPDocs? Are all parameters, return values, etc. commented?
3. Is all complex or non-obvious logic commented?
5. Is the logic as simple as it can be?
5. Are all coding standards enforced?
6. Are Joomla patterns consistently implemented?
7. Are methods short?
8. Are abbreviations minimized and consistent?
9. Do PHPDocs include examples?
10. Are unit tests thorough?
...
n. If all non-optional critiques have not been addressed the code should not be merged to production.

And so on. You'd want to have a comprehensive checklist.

Code reviews are mostly about the things that automated testing and linting tools can't catch. But you definitely still want to use those tools too!

I recommend using a code review tool. There are various free and/or open source tools. At work I use Crucible, which isn't free.

[Mark Dexter]

Wow, what a great checklist! For now, I think I'll start a wiki page so I can try to remember all of these as I write a read code. Thanks. Mark

[Niels Braczek]

On 08.10.2011 14:18, Gary Glass wrote:

> Definitely something else. Good professional-quality code reviews include things like:

> [...]

> n. If all non-optional critiques have not been addressed the code should not be merged to production.
> And so on. You'd want to have a comprehensive checklist.

Would be great, if we reached that level of quality assurance.

> I recommend using a code review tool. There are various free and/or open source tools. At work I use Crucible, which isn't free.

Do you have examples of good OS tools?

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

[Gary Glass]

It’s just a start, of course, off the top of my head. (Can you tell I’ve done a few code reviews?)

 

______________________________

Gary Glass

http://ShutterGlass.com

http://OnlyBegotten.com

 

[Gary Glass]

The only tool I've used is Crucible. But here's something I googled:
http://ostatic.com/blog/open-source-code-review-tools

The first one, Rietveld, looks promising. It appears to be backed by Google.

______________________________
Gary Glass
http://ShutterGlass.com
http://OnlyBegotten.com

-----Original Message-----
From: joomla-dev...@googlegroups.com
[mailto:joomla-dev...@googlegroups.com] On Behalf Of Niels Braczek
Sent: Saturday, October 08, 2011 11:41 AM
To: joomla-dev...@googlegroups.com
Subject: Re: [jframe] Coding standards request

On 08.10.2011 14:18, Gary Glass wrote:

> Definitely something else. Good professional-quality code reviews include
things like:
> [...]
> n. If all non-optional critiques have not been addressed the code should
not be merged to production.
> And so on. You'd want to have a comprehensive checklist.

Would be great, if we reached that level of quality assurance.

> I recommend using a code review tool. There are various free and/or open
source tools. At work I use Crucible, which isn't free.

Do you have examples of good OS tools?

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

[Niels Braczek]

On 08.10.2011 18:51, Gary Glass wrote:

> The only tool I've used is Crucible. But here's something I googled:
> http://ostatic.com/blog/open-source-code-review-tools

Thank you, that page is a good starting point.
Seems like I've to do some investigations...

Regards,
Niels

--

[Niels Braczek]

On 08.10.2011 19:21, Niels Braczek wrote:

> Seems like I've to do some investigations...

Just stumbled upon
http://source.android.com/source/life-of-a-patch.html. It shows the
workflow using Gerrit, a web-based code review tool, which integrates
nicely with Git.

[Gary Glass]

I also see that crucible is free for open source projects. I think it's an excellent tool.

http://www.atlassian.com/software/crucible/pricing.jsp

__________________________
Gary Glass

[elin]

As the person who spent 4 months filling in a lot of missing docblck, I can tell you that all methods have doc blocks and no methods will get in without them. All arguments are defined as are all returns, those are picked up as part of the automated code sniffing.

You can see that Andrew has built an infrastructure for examples in the docs portions of the code base but the big project we need to do is to have everyone work backwards from the tests (where they exist, writing tests where they don't) to provide examples for each method. I've thought some about how to make this happen. One idea I have is to have a study group of people who would like to improve their framework knowledge and would perhaps like to work together on a class a week, writing the examples as we go. Would there be interest in that? Would people be willing to commit to say 4 week stints of working through the classes until we are done?

Elin

[tomfuller]

Elin, I think that is a terrific idea that I would be interested in. My biggest drawback with writing code for Joomla! extensions is a lack of experience knowing the classes and methods of the framework. If you decide on the class idea, I'd like to be involved!

Tom

[Andrew Eddie]

Nice list!

On 8 October 2011 22:18, Gary Glass <ga...@mccull.org> wrote:
>
> 1. Do all non-private members have phpdoc comments?

We actually required private methods and properties to be fully
documented. You still have to work out what they do if you aren't the
author :)

> 2. Is the commenting correct? Does it generate correct PHPDocs? Are all parameters, return values, etc. commented?

We don't check the generation but we do check comments have been
written and Code Sniffer also does an automated test.

> 3. Is all complex or non-obvious logic commented?

Our CI server does a test for this but our priority has been getting
the Code Sniffer warnings down.

> 5. Is the logic as simple as it can be?

If it's well tested, that usually follows. Unfortunately this can be
arbitrary and spending too much time on it can use up valuable
volunteer time.

> 5. Are all coding standards enforced?

PHP CodeSniffer

> 6. Are Joomla patterns consistently implemented?

Probably something for group review as Elin suggests.

> 7. Are methods short?

Messiness detector dows a report for this.

> 8. Are abbreviations minimized and consistent?

Good thought.

> 9. Do PHPDocs include examples?

If the are short I think that's fine, but we have a repo for more
complex examples.

> 10. Are unit tests thorough?

Code coverage reporting already helps with that.

Our CI server is built on Jenkins.

[Niels Braczek]

On 08.10.2011 19:21, Niels Braczek wrote:

> Seems like I've to do some investigations...

Look at this article:
http://benjamin-meyer.blogspot.com/2008/08/code-review-should-be-free.html
It explains how to use GitHub for code review (at the very end of the
article), and also covers alternatives.

[Gary Glass]

Like I said earlier, these automated tools are great, and I wholeheartedly support their use (wish I could convince my company to use them more), but code reviews are really about doing all the things that tools can't. A sniffer can tell you a method is more than 20 lines, but reviewers can help you figure out how to make it shorter, simpler, better. As a reviewer one of the things I focus on is making the code as easy as possible to read. Coders often focus on technical concerns like performance, footprint, etc. These are important. Reviewers should also be thinking about those things. But reviewers because they do not know this code are naturally disposed to think about readability. That goes to reducing maintenance overhead, future enhancements, ease of adoption, and so on. These are human factors issues. The Joomla community is up against some of these problems now. 3rd party extensions rely on APIs and design decisions in the framework that we might have done differently if… But now we need to maintain them for compatability. This is a typical and inevitable problem for any framework. Code review is one way to fight against this entropy.

__________________________
Gary Glass

[elin]

Gary,

These are important. Reviewers should also be thinking about those things.

"Reviewers" are people who review

It sounds like you should be in there doing code review. If it is something you are passionate about, don't wait for an invitation or permission, go over to github and start reviewing and making pull requests. Don't for a second worry about if other people care about an issue, if you care about it it is worth your time to work on it.  Certain people make fun of me for fixing lets and let's in the comments, but I know that they mean two totally different things. 

Elin

[Gary Glass]

Yes and if these comments are generating your public API documentation, grammatical errors in those docs just make Joomla look amateurish.

__________________________

Gary Glass

--

You received this message because you are subscribed to the Google Groups "Joomla! Framework Development" group.

To view this discussion on the web, visit https://groups.google.com/d/msg/joomla-dev-framework/-/k-iZ1plG5nkJ.

[Rafael Corral]

I completely agree with you Elin.
I just wanted to make sure I wasn't going to put work in for changing
these tabs to spaces and then have my pull request denied because
those are not coding standards.

Also, I would be interested in the classes idea. I am pretty familiar
with the framework as the only way I learn is by going through it, but
I do think that there needs to be more documentation in some way or
another to help newcomers learn the framework. I am willing to help
out if needed.

Rafael

On Oct 9, 9:53 pm, elin <elin.war...@gmail.com> wrote:
> Gary,
>
> These are important. Reviewers should also be thinking about those things.
>
> "Reviewers" are people who review
> It sounds like you should be in there doing code review. If it is something
> you are passionate about, don't wait for an invitation or permission, go
> over to github and start reviewing and making pull requests. Don't for a

> second worry about if other people care about an issue, if *you *care about

> it it is worth your time to work on it.  Certain people make fun of me for

> fixing lets and let's in the comments, but *I *know that they mean two
> totally different things.
>
> Elin

[Gary Glass]

+1

I would like to help with docs too. I'm no framework expert but I am a writer. I think documentation is Joomla's weakest link. I cannot emphasize enough how vital documentation is to improving the product and extending it's reach.

__________________________
Gary Glass

> --
> You received this message because you are subscribed to the Google Groups "Joomla! Framework Development" group.

[Andrew Eddie]

There are currently two ways we document.

The first is through the compilation of a "programmers manual" and
this can be found in /docs/. The file format it Docbook and there are
a number of tools around to help you edit them. I use XML Mind myself.

The second is by way of example, so if you have some good examples you
want to share, shoot me a pull request on this repo:
https://github.com/joomla/joomla-platform-examples

I suggest you open up a new thread if you want to discuss documentation more.

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla 1.7 developers