Unify commenting style in the project #609

Closed
pascalchevrel opened this Issue Feb 5, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@pascalchevrel
Member

pascalchevrel commented Feb 5, 2016

PHP allows commenting code in many ways borrowed from other languages and there isn't established best practices set in the PSR recommendations yet.
There is:

// foo

# foo

/* foo */

/*
 * foo
 */

/*
foo
*/

The only established reference is for docblocks which imposes a specific syntax and 2 asterisks at the beginning of the comment to define that this is extractable code documentation for text editors and documentation generators:

    /**
     * Get the list of parameters for an API call.
     *
     * @param  string $parameters The list of parameters from the URI
     * @return array  All the main parameters for the query
     */

This one is a no-brainer as most IDEs/Editor allow inserting a docblock automatically.

For long comments the formatting I prefer is this one as it is I think clearer for reading the text itself:

    /*
        We are only interested in target strings with keys in common with our
        source strings. Not sending noise to getTranslationMemoryResults() has
        a major performance and memory impact.
    */

That is to say no mid-asterisks and an indented paragraph to make the text stand out.

For a one line comment, personally I think I almost never use bash-style comments, actually I do but only during development for broken or incomplete stuff to find it easily with grep:

#TODO: do something
#FIXME: do something
#LATER: do something

For one line comments, currently most of our code base uses //:

// I am a comment

I think this is OK especially since it seems to be the default in popular text editors I know with PHP when you comment out a line (tested in Atom and Sublime Text ).
But sometimes we mix styles ex:

        // Extract, cache and store VCS data
        $cache_id = $repo_vcs . $locale . 'healthstatus2';
        if (! $stats = Cache::getKey($cache_id)) {
            /* generate data */
            $commits = $vcs->getCommits();

Or we use slashes for a multiline comment:

// Set CSS classes
// If the latest commit is 1 year old or older

So here is my proposal to make it consistent:

  • Never use #
  • Use // only for a one line comments to explain logic in the code (note the space after the //)
  • Use this for general documentation and longer logic explanations:
/* 
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor 
    incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud
    exercitation ullamco laboris nisi ut ali
*/
  • If your comment explains a logic block or a loop (if, for, foreach, switch…), put it BEFORE the block and not inside, like that:
    // build the source locale switcher
    $source_locales_list[$repository] = Utils::getHtmlSelectOptions(
        $loc_list[$repository],
        Project::getLocaleInContext($source_locale, $repository)
    );

    // build the target locale switcher
    $target_locales_list[$repository] = Utils::getHtmlSelectOptions(
        $loc_list[$repository],
        Project::getLocaleInContext($locale, $repository)
    );

    // 3locales view: build the target locale switcher for a second locale
    $target_locales_list2[$repository] = Utils::getHtmlSelectOptions(
        $loc_list[$repository],
        Project::getLocaleInContext($locale2, $repository)
    );

The reason for this last rule is that if you collapse all blocks in your code and your code is well commented you can read the logic of the file by reading the comments, otherwise they are hidden:

    // build the source locale switcher
    $source_locales_list[$repository] = Utils::getHtmlSelectOptions( ¬ 

    // build the target locale switcher
    $target_locales_list[$repository] = Utils::getHtmlSelectOptions( ¬ 

    // 3locales view: build the target locale switcher for a second locale
    $target_locales_list2[$repository] = Utils::getHtmlSelectOptions( ¬ 

@flodolo @TheoChevalier since you are the two other main contributors, would you agree with these conventions? If that is the case, we could turn it into a real issue for the Locasprint, this is kind of tedious work but I think that for a new contributor, it can help learn our whole code base and file structure.

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Feb 5, 2016

Contributor

Sounds good to me. I would also add one more note: start the comment with uppercase, don't end with a period (maybe with the exception of multiline comments).

Contributor

flodolo commented Feb 5, 2016

Sounds good to me. I would also add one more note: start the comment with uppercase, don't end with a period (maybe with the exception of multiline comments).

@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Feb 5, 2016

Member

+1 for uppercase and a period only for ending multiline comments

Member

pascalchevrel commented Feb 5, 2016

+1 for uppercase and a period only for ending multiline comments

@TheoChevalier

This comment has been minimized.

Show comment
Hide comment
@TheoChevalier

TheoChevalier Feb 5, 2016

Member

I'm also fine with this new comments styleguide :)

Member

TheoChevalier commented Feb 5, 2016

I'm also fine with this new comments styleguide :)

@pascalchevrel

This comment has been minimized.

Show comment
Hide comment
@pascalchevrel

pascalchevrel Feb 12, 2016

Member

Once this is done, these instructions should be added to the wiki https://github.com/mozfr/transvision/wiki/Code-conventions

Member

pascalchevrel commented Feb 12, 2016

Once this is done, these instructions should be added to the wiki https://github.com/mozfr/transvision/wiki/Code-conventions

Thegennok added a commit to Thegennok/transvision that referenced this issue Feb 14, 2016

Thegennok added a commit to Thegennok/transvision that referenced this issue Feb 14, 2016

Thegennok added a commit to Thegennok/transvision that referenced this issue Feb 14, 2016

flodolo added a commit that referenced this issue Feb 14, 2016

Merge pull request #627 from Thegennok/comment
Comment modification for following rules elaborate in #609
@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Feb 14, 2016

Contributor

Most of the comments should be fixed now.

@Thegennok
Are you able to update https://github.com/mozfr/transvision/wiki/Code-conventions?

Contributor

flodolo commented Feb 14, 2016

Most of the comments should be fixed now.

@Thegennok
Are you able to update https://github.com/mozfr/transvision/wiki/Code-conventions?

@Thegennok

This comment has been minimized.

Show comment
Hide comment
@Thegennok

Thegennok Feb 14, 2016

Contributor

Yup, just made the change :)

Contributor

Thegennok commented Feb 14, 2016

Yup, just made the change :)

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Feb 14, 2016

Contributor

Looks great, thanks. Did some minor styling fixes to the rest of the doc, but looks good.

Contributor

flodolo commented Feb 14, 2016

Looks great, thanks. Did some minor styling fixes to the rest of the doc, but looks good.

@flodolo flodolo closed this Feb 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment