Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Propel2 Coding Standards #2

Closed
willdurand opened this Issue · 50 comments
@willdurand
Owner

Propel2 Coding Standards

We use the Symfony2 convention, here is a short example (from http://symfony.com/doc/current/contributing/code/standards.html

<?php

/**
 * This file is part of the Propel package.
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 *
 * @license    MIT License
 */

namespace Propel;

use A\Baz;
use B\Bar;

class Foo
{
    const SOME_CONST = 42;

    private $foo;

    /**
     * @param string $dummy Some argument description
     */
    public function __construct($dummy)
    {
        $this->foo = $this->transform($dummy);
    }

    /**
     * @param string $dummy Some argument description
     * @return string|null Transformed input
     */
    private function transform($dummy)
    {
        if (true === $dummy) {
            return;
        }
        if ('string' === $dummy) {
            $dummy = substr($dummy, 0, 5);
        }

        return $dummy;
    }
}

Structure

  • Never use short tags (<?);
  • Don't end class files with the usual ?> closing tag;
  • Indentation is done by 4 spaces, don't use tabs;
  • Use the linefeed character (0x0A) to end lines;
  • Add a single space after each comma delimiter;
  • Don't put spaces after an opening parenthesis and before a closing one;
  • Add a single space around operators (==, &&, ...);
  • Add a single space before the opening parenthesis of a control keyword (if, else, for, while, ...);
  • Add a blank line before return statements, unless the return is alone inside a statement-group (like an if statement);
  • Don't add trailing spaces at the end of lines;
  • Use braces to indicate control structure body regardless of the number of statements it contains;
  • Put braces on their own line for classes, methods, and functions declaration;
  • Separate the conditional statements (if, else, ...) and the opening brace with a single space and no blank line;
  • Declare visibility explicitly for class, methods, and properties (usage of var is prohibited);
  • Use lowercase PHP native typed constants: false, true, and null. The same goes for array();
  • Use uppercase strings for constants with words separated with underscores;
  • Define one class per file;
  • Declare class properties before methods;
  • Declare class visibility after static or final keywords (e.g. final public, static public, final static, ...);
  • Organize use alphabetically and group them by Runtime, Generator, and others. Each group will be separated by a new blank line.

Naming Conventions

  • Use camelCase, not underscores, for variable, function and method names;
  • Use namespaces for all classes;
  • Use Propel as the first namespace level;
  • Suffix interfaces with Interface;
  • Use alphanumeric characters and underscores for file names;
  • Don't forget to look at the more verbose Conventions document for more subjective naming considerations.

Documentation

  • Add PHPDoc blocks for all classes, methods, and functions;
  • Omit the @return tag if the method does not return anything;
  • The @package and @subpackage annotations are no more used.

License

The following license has to be included in all files:

/**
 * This file is part of the Propel package.
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 *
 * @license    MIT License
 */
@fzaninotto
  • Indentation is done by tabs +1
  • Use underscores for option, argument, parameter names - any example?
  • Declare public methods first, then protected ones and finally private ones; - Why? It often makes sense to put the protected methods called by public ones just after the public methods.
  • Use Symfony as the first namespace level; => Use *Propel** as the first namespace level;*
@willdurand
Owner
  1. I don't like the indentation with tabs…
  2. $options['foo_bar']
  3. Actually, I'm removing it as it will be impossible to respect this rule, too much existing code.
  4. Thanks!
@fzaninotto
  • Then let's go for indentation with spaces, you're the boss :) 2 spaces? 4 spaces?
  • I think we should use camelCase all the time in PHP to avoid confusion. underscore should be limited to the database level
@havvg

[x] Vote 4 spaces.
As Propel2 will be closely bound to Symfony community, the CS should not differ.

Namespacing for options is done by a . (dot)?

<?php // wild example
$options['acme.mystuff.use_foo'];
@jaugustin
Collaborator
  • 4 spaces (sf2 convention)
  • I agree with the all camelCase :)
@vworldat

As much as I don't like spaces for indentation, I have to agree that uniformity is useful, so +1 for 4 spaces.
Everything else looks nice for me so far, and yes, I also disapprove ordering methods by visibility, I much prefer ordering/grouping by usage, which makes understanding big classes (think generated base models) much easier.

@stof

just a point about your example. Using the Sf2 standards, it should be

<?php
    private function transform($dummy)
    {
        if (true === $dummy) {
            return;
        }
        if ('string' === $dummy) {
            $dummy = substr($dummy, 0, 5);
        }

        return $dummy;
    }

else is not used when the if returns.

@willdurand
Owner

Oh you're right, so there is the same problem in the doc.

@casivaagustin

About the Spaces:

Pear Uses 4 spaces
Zend Uses 4 spaces
Drupal Uses 2 spaces
Symfony USes 2 spaces

4 spaces, in my Opinion, it's too much .... with 2 spaces you are able to write more code in 80 cols. And if you make
a line break usually we use a double identantion, that will represent 8 spaces :S.

@stof

Symfony2 uses 4 spaces

@willdurand
Owner

Yeah, 4 spaces is the best choice, this is also Oracle CS.

@casivaagustin

Doctrine uses 4 too :S .... maybe the best it's use 4

@dguyon

+1 to use camelCase only
Don't see much interest in grouping methods by visibility too

@cedriclombardot

+1 for CamelCase

I think 4 spaces is better. Like the new frameworks

And for the @stof sample :

<?php
private function transform($dummy)
{
if (true === $dummy) {
return;
}
if ('string' === $dummy) {
$dummy = substr($dummy, 0, 5);
}

    return $dummy;
}

I think that miss à line between the 2 conditions

@willdurand
Owner

I updated CS, it should be ok now.

@Xosofox

+1 for 4 Spaces

I switched myself from 2 to 4 when I heard that Symfony2 will use 4 - and meanwhile I don't like the looks of "old" symfony1-code with 2 spaces anymore

@hhamon
Collaborator

+1 for 4 spaces (better compatibility with IDEs, avoid dummy conflict statuses with SCM tools)
+1 for camel case

@garak

+1 for 4 spaces

@hhamon
Collaborator

static public function doSomething() or public static function doSomething() ?

@stof

symfony2 uses static public because the static keywords gives more information about this method than the public one (having public methods is the standard way) so putting it first makes it more obvious when reading the code. for the same reason, we use final public over public final (the order is undefined for methods that are both final and static. @fabpot do you have some details on this case ?).

@fabpot

@stof: final static is pretty rare but personally, I would prefer final static (don't ask me why ;)).

@hhamon
Collaborator

I'm also in favor or static and final keywords first.

@willdurand
Owner
@jaugustin
Collaborator

I think we should remove Propel prefix from classnames because it's already in the namespace.

@willdurand
Owner
@apinstein

Why isn't this doc in the root of the project? Wouldn't it make more sense in there so it can't be missed by devs?

Also, how do you do if and switch?

@stof

@apistein because it is not fully ready for the doc yet. It is discussed so that everybody agree on it

@apinstein
@fzaninotto

One general question about class naming. Imagine a Memcached adapter for a function cache. Should the class be named:

MyVendor\Function\Cache\Adapter\Memcached

or

MyVendor\Function\Cache\Adapter\MemcachedAdapter

In other terms, do we repeat the last part of the namespace in the class name? This is what Symfony2 does, but it's not what Doctrine2 and Zend Framework 2 do.

Personally, I prefer less verbosity, and I consider that the actual class name is the fully qualified class name, so there should be no repetition in the fully qualified class name (otherwise we don't use namespaces). In short, I prefer the first version.

Opinions?

@willdurand
Owner

I don't like the first version actually.

The benefit of the second version is to be explicit in the code. Memcached is quite clear but how about Array ?
It could be:

  • a formatter;
  • a collection;
  • a cache adapter;

In short, I prefer the second version. If you can prove me that the first version is much better than the second one, let's go for the first version. Personally, I like to read the Symfony2 code because it's like a book, you don't turn the previous page to know who is who.

@stof

Array would simply be forbidden by PHP as it is a reserved keyword

@fzaninotto

I like browsing the Doctrine2 and Zend FW2 directory structure. It makes sense. FQCN are already very verbose, I see the repetition in them as a burden. But let's see what other think.

@jaugustin
Collaborator

I think the first version is good but we should use the second one in particular case, when the last part of the FQCN is not explicit.

@willdurand
Owner

Ahm, @Stof got me but I still don't like the first version. We are all agree that FQCN isn't a problem anyway.

@willdurand
Owner

No, we won't use both. My previous example was a real use case.

@fabpot

In Symfony2, we always use short class names in the code as we add use statements at the beginning of the file. That way we have nice looking code... but that's only true if the short class names convey enough information about the class being used. Request is probably enough for a web framework and does not any other information added to it (like in WebRequest) but what about a Yaml Loader? What would be the name? In Symfony2, it would probably be something like Propel\Loader\YamlLoader and in the code you would instantiate it like this: $loader = new YamlLoader();. But if you don't repeat Loader, it makes less sense: $loader = new Yaml();. In this case, it would be better to just use the FQCN or at least the last segment of the namespace as in: $loader = new Loader\Yaml();. For common names, it's even more problematic as you then need to rename the class to avoid collisions. AFAIK, Java also tries to come up with meaningful class name (without the package name).

@fzaninotto

@fabpot: Makes sense.

@willdurand
Owner

So the problem is to choose between new Loader\Yaml() and new YamlLoader()...

@hhamon
Collaborator

use Propel\Loader\Yaml as YamlLoader

:)

@jaugustin
Collaborator
@fzaninotto

+1 for use Propel\Loader\Yaml as YamlLoader.

So William, what do you decide?

@hhamon
Collaborator

I have just noticed that in Symfony we have :

Symfony\Component\Routing\Loader\XmlFileLoader;
Symfony\Component\Routing\Loader\YamlFileLoader;

So maybe we can follow the same direction.

Propel\Loader\YamlLoader

It's probably more concise and meaningful than

Propel\Loader\Yaml as YamlLoader

My 2 cents.

@dguyon

+1 for new YamlLoader()
Makes more sense as @hhamon said

@K-Phoen

I also think that the Propel\Loader\YamlLoader is clearer.

+1 for @hhamon's last proposition

@willdurand
Owner

@fzaninotto : I cannot agree with you. If we start to use as on all use statements, then it will be a mess in a couple of months.

I'm still +1 for naming things like Symfony2 does. That means, it will be YamlLoader. But in another hand, we don't apply this rule for all stuffs, Model in generator for instance. We won't rename classes from Table to TableModel, it doesn't make sense.

Yaml can be a dumper, a formatter, a loader, and so on. But it's quite different with Table. So I suggest to name things explicitely, even if it's not short, and I suggest to try to avoid duplicate class names in the same top-level namespace (aka Propel\Generator, Propel\Runtime).

Suggestions?

@ludofleury

Late, but YamlLoader is much more convenient than as

@fzaninotto

Can we closes this issue and make it a page in the wiki/in the doc?

@willdurand
Owner
@fzaninotto

Ok. You opened it, I'll let you close it :)

@willdurand willdurand closed this
@willdurand willdurand referenced this issue
Closed

Fix CS #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.