Formulate guidelines for phpdoc and inline comments

[fixpunkt]

Proposal:

• Classes, properties and methods should be documented.
• Always use a single-sentence short description for classes and methods.
• Single-sentence short description and the long description (if one exists) are paragraphs.
• Separate paragraphs using blank lines, not single linefeeds.
• @param, @throws and @return descriptions should always include types, not start with a capital letter and not end in punctuation, unless they're whole sentences consisting of subject, verb and object (which they usually aren't and that's fine).
• @param and @return should not repeat the subject that is already defined by the type, i.e. they should be sentence fragments
• Avoid You and I, we only if necessary
• Avoid "cleverness" and humor
• Avoid redundant comments
• For methods, classes and properties, a redundant comment is one that just repeats the name of the documented thing with a few filler words or spaces added.
• A redundant inline comment is one that just says what the next statement does (you should instead refactor the statement to explain what it does, by using appropriate variable and method names)
• In general, inline comments should explain the bigger context, the "why" of things or non-obvious stuff such as performance optimizations or workarounds for bugs in frameworks. If you're simply explaining what your code does, you're doing it wrong
• Misleading comments are a hazard. When you find a comment that was not updated along with the code it explains, radically removing it instead of fixing it is an option, too.
• Removing redundant comments is a civic duty
• Commented-out is never committed.
• Open tasks go in the issue tracker, never in the code (TODO, FIXME, ...). It's best to not even commit them locally or on your feature branches, because these things tend to be forgotten (which is exactly why they're unsuitable for tracking issues). We should probably have a static check for this in our pre-commit hook.

[cjuner]

I really like your list!

• WRT: Comment out-code is never committed.
  Absolutely agree, if it is "code". I think we should make it clear that usage examples for codes are welcome in the method's/function's/class's doc block.

• WRT: Classes, properties and methods should be documented.
  I propose that this should only be the case when the comment explains more than the entity's name would. E.g., completely self-descriptive class or method names like getFoo or loadXFromDb probably need no explanation. I propose, in such cases, we favor less scrolling over redundant information. In general, I propose we suggest naming things so fewer additional comments are required.

• WRT: No TODO, FIXMEs in committed code
  I completely disagree. There are some issues that are such low-priority and are much easier to understand if the affected code mentions possible improvements. Some of them might not be worth tracked issues, because they might only become relevant when the code is reworked or refactored.

I agree with the rest.

[windaishi]

Classes, properties and methods should be documented.

And constants

@param, @throws and @return descriptions should always include types, not start with a capital letter and not end in punctuation, unless they're whole sentences consisting of subject, verb and object (which they usually aren't and that's fine).
@param and @return should not repeat the subject that is already defined by the type, i.e. they should be sentence fragments

I do NOT agree here.

This is how a @param tag looks line in a PHPDoc:

    /**
     * @param string $path to directory
     * @param string $path2 Path to directory
     * @param string $path3 The Path to the directory.
     * @param \Exception|null $previousException
     */

So I suggest using the following rules:

• Neither type nor $variable is part of the parameter description
• Parameter description should always start with a capital letter.
• Parameter descriptions can be a sentence.

Example doc block:

/**
 * Foos the $bar with some spicy chicken.
 *
 * While fooing, no other Bar can be fooed. If you try this, most of 
 * the bars will just ramdomly explode.
 * Also $bar must be type of deer or mushroom.
 * I won't tell you anthing here how the fooing of the $bar works in 
 * detail.
 *
 * @param object $bar A $bar to foo.
 * @return int Number of foos the $bar got.
 * @throws \FooException if the $bar does not want to be fooed.
 */
public function foo(object $bar) {
    if ($bar->some) {
        throw new \FooException();
    }

    return 1;
}

Also I strongly suggest to use Markdown in PHPDoc descriptions.

[fixpunkt]

Neither type nor $variable is part of the parameter description
Yup, I agree. The example I gave was stupid and it's probably better to keep this separate, in particular when looking at how this is shown in the generated html.

Parameter descriptions can be a sentence.
I agree, but please keep in mind that in 95% of cases (a statistic I just pulled out of my arse), this is not the case. Adding a period to a term doesn't make it a sentence, e.g.

* @param string $path3 The Path to the directory.

This is not a sentence and there should be no period at the end. Sentences have verbs.

Parameter description should always start with a capital letter.

With regard to the starting capital letter or not question, I've had a look at what other people do. In the Java world, parameters descriptions start in lowercase. This is probably one of the reasons I prefer this format and why the capital letter looks very weird to me. I've also had a look at PHP projects. Symfony and Composer both start in a capital letter, which makes sense if you think of the description part as a table cell as it's shown in PhpDocumentor-generated output. This is probably also why they column-align their param lists. This doesn't look as weird as having a capital right in the middle of a line.

Since I seem to be the only one who seems to prefer to avoid starting in uppercase letters, I'm ok with making uppercase letters a requirement. But I propose if we agree on that, we should column-align the parameter list the same way Symfony and Composer do (and Shopware, too, btw).

One thing I dislike about @windaishi's example is this:

 * While fooing, no other Bar can be fooed. If you try this, most of 
 * the bars will just ramdomly explode.
 * Also $bar must be type of deer or mushroom.
 * I won't tell you anthing here how the fooing of the $bar works in 
 * detail.

This looks too random. We should only use real paragraphs (separated by blank lines), not half-paragraphs (i.e. single linefeeds), and instead wrap all text within a paragraph to 120 cols. Otherwise, you end up with two different levels of paragraph demarcations, which is just too distracting to read. Also, I'm pretty sure those linefeeds won't be in the generated PhpDocumentor output.

[cjuner]

I slightly prefer lowercase for parameter descriptions because they rarely are complete sentences. I could live with uppercase, though.

[windaishi]

This is how it looks in the QuickDoc view of PHPStorm.

In my opinion, another argument towards uppercase.

[windaishi]

This looks too random. We should only use real paragraphs (separated by blank lines), not half-paragraphs (i.e. single linefeeds), and instead wrap all text within a paragraph to 120 cols. Otherwise, you end up with two different levels of paragraph demarcations, which is just too distracting to read. Also, I'm pretty sure those linefeeds won't be in the generated PhpDocumentor output.

Good point. Anyway you can format your description with markdown (and especially github-flavoured markdown).

[windaishi]

No TODO, FIXMEs in committed code

I completely disagree. There are some issues that are such low-priority and are much easier to understand if the affected code mentions possible improvements. Some of them might not be worth tracked issues, because they might only become relevant when the code is reworked or refactored.

I aggree with @fixpunkt. No TODOs and FIXMEs in code.

TODOs are only for the developement process and should never be commited to master. Actually TODO means (imho) "I'm currently too lazy TODO it now, so I wait for someone other TODO it".

FIXME is always an issue. I don't know anything that needs to be fixed and is not worth an issue.

[cjuner]

This is one example of what TODOs can be used for: https://github.com/VIISON/ShopwareCommon/blob/bf6686698796c5af29c7c6bced8f0a57ee77ef67/Classes/TryWithFinally.php#L14

Are you really suggesting we create an issue for any such instance? What exactly is it that you're proposing? I think grepping the code for TODOs/@todo/FIXME every once in a while is much less heavyweight than a filled issue tracker full of unimportant issues.

Please only require column alignment of docs if a hook can do it. Though I prefer indented multi-line descriptions where supported. Column-aligning has the tendency to make lines really long.

[windaishi]

Okay, that special case is different. This is not one of the classic TODOs order FIXMEs. We should then allow @todo as PHPDoc tag.

[fixpunkt]

Are you really suggesting we create an issue for any such instance? What exactly is it that you're proposing? I think grepping the code for TODOs/ every once in a while is much less heavyweight than a filled issue tracker full of unimportant issues.

It's an opaque extra process, issues are visible to everyone. And if our issue tracker is full of refactoring issues, that is very important to know so we can make appropriate plans to remove technical debt.

[cjuner]

It's an opaque extra process, issues are visible to everyone. And if our issue tracker is full of refactoring issues, that is very important to know so we can make appropriate plans to remove technical debt.

In my example, grepping for something like '@todo.*PHP.*5.5' reveals all related TODOs. How would a corresponding issue look? Manually link each line of code that could be changed? I propose we at most create an issue with strings to grep for :)

[fixpunkt]

That's still not visible to the half of the company that doesn't grep.

[cjuner]

So what's the alternative? Manually maintain dozens of Markdown links to specific lines of code?

[fixpunkt]

Just not committing half-assed code should be the best course of action for most cases where people use TODOs. If it's something you cannot fix right now (like your example), make an issue and add a GitHub permalink (no markdown required). From experience, that happens rarely enough to not be a problem.

[cjuner]

What's half-assed about my example about a TODO to change code once we can depend on PHP >= 5.5? Please be really specific how you would handle this case. I haven't seen a reasonable alternative approach for this example, yet.

Make a permalink to what?

[fixpunkt]

I explicitly stated that the half-assedness refers to most cases, but not to your example.

You can just link the GitHub blob like you did in your commenbt above: https://github.com/VIISON/ShopwareCommon/blob/bf6686698796c5af29c7c6bced8f0a57ee77ef67/Classes/TryWithFinally.php#L14

Put that in an issue, add the refactoring label and you're done. This really doesn't happen so much that it's a problem, that's how we've been handling this case for some time now.