Fix coding style (static analysis)

[nodiscc]

A pass of an automatic coding style fixer may be needed to make shaarli more maintainable.
This will help find and trim down unnecessary complexity/broken code.

[nodiscc]

I think this should be the first thing to do once 0.9 beta is released. There are several style fixers available, https://github.com/FriendsOfPHP/PHP-CS-Fixer is one.

[virtualtam]

A good start would be to submit the PHP code to a static code analyzer (here's an article listing some).

What is likely to be detected:

• code formatting:
  • indentation,
  • lines too long (>80 chars),
  • parenthesis, commas, (trailing) spaces, tabs...
• variables assigned and never used,
• functions/methods:
  • unused arguments,
  • complexity (# variables, # statements).

Benefits of static analysis:

• functionality: almost none;
• development:
  • helps defining a shared coding convention,
  • spot typos, undefined variables and other editing glitches,
  • when the # of detected issues has become low, instant spotting of typos!

How you can encourage devs to use it:

• Makefile with shortcuts to testing tools:
  • static analysis,
  • dynamic analysis,
  • unitary tests,
  • functional tests;
• Travis setup to run tests after a commit is pushed on GH.

A first run of PHP Code Sniffer on index.php returns 900+ errors and warnings, most of them being valid:

 2601 | ERROR   | [ ] Expected "if (...) {\n"; found "if (...) { "
 2601 | ERROR   | [x] No space found after comma in function call
 2601 | WARNING | [ ] Line exceeds 85 characters; contains 110
      |         |     characters
 2601 | ERROR   | [x] Closing brace must be on a line by itself
 2602 | ERROR   | [ ] Expected "if (...) {\n"; found "if (...) { "
 2602 | ERROR   | [x] No space found after comma in function call
 2602 | ERROR   | [x] Closing brace must be on a line by itself
 2602 | WARNING | [ ] Line exceeds 85 characters; contains 142
      |         |     characters
 2603 | WARNING | [x] Inline control structures are discouraged
 2603 | WARNING | [ ] Line exceeds 85 characters; contains 106
      |         |     characters

Regarding code "fixers", I'm not a huge fan of having a tool altering a codebase by itself... sure, doing this by hand is tedious, but browsing your code is also a good way to spot bugs and inconsistencies (and learn things, too) :-)

[nodiscc]

I agree that "fixer" changes are not easy to review, we should be wary of automatic linting tools. Thanks for pointing to PHP Code Sniffer, it's something that can definitely be done (it doesn't have to be done all at once, small improvements and patches are welcome). I'll have a look at it.

[virtualtam]

A common usage is to fix warnings within / close to commit scopes, e.g.

• I'm working on a feature that requires modifying function1() and function2(),
• I'll submit a patch that contains:
  • functional changes (what's needed for the feature),
  • code cleanup for the two modified methods.

This, or a "code cleanup rampage" sprint ;-)

I should have some spare time in the upcoming weeks to look at this, plus #71; both being code-quality-related.

[e2jk]

Great suggestions. +1 for non-automatic changes (well, I guess I don't care if it's automatic, as long as if it's not one big commit in git. If you find a way to have an automatic tool change only one aspect at a time, it makes it much more easy to review). Having a Continuous Integration (e.g. TravisCI) would be great. When submitting pull requests to Ampersand.js, I really like to see the messsage indicating that the specific change hasn't yet been tested/passed CI validation.

[nodiscc]

I get a lot of these errors using php-cs. What is expected/how to fix this?

120 | ERROR | [ ] You must use "/**" style comments for a function comment

There is no comment on line 120...

[nicolasdanelon]

@nodiscc you just run make ? or something else?

[nodiscc]

make code_sniffer_full

[virtualtam]

This warning points to the multi-line comment on lines 116-118, which may be associated by CodeSniffer with the function declaration at line 120:

//==================================================================================================
// Checking session state (i.e. is the user still logged in)
//==================================================================================================

function setup_login_state() {
    [...]
}

which could be replaced by:

/**
 * Checking session state
 *
 * A bit more comments on what the function performs
 */
function setup_login_state() {
    [...]
}

[nodiscc]

Thanks @virtualtam, I will take care of these. Start small.

[virtualtam]

You're welcome :)

We can safely start with fixing:

• lines too long,
• comment formatting,
• indentation / extra of missing spaces / bracket stuff.

This will remove a lot of noise from the output of code checkers, and help spot more important issues.

(Bonus section)
Regarding comments, it could be neat to settle on a Doxygen-like formatting (dunno if there are other tools more suitable for PHP), e.g.

/**
 * Brief description of the function
 *
 * Longer description of the function (optional):
 * - it does this,
 * - and that,
 * - not to mention it's also good at doing this!
 *
 * @param $p1  Param description
 * @param $p2  Param description
 * @return What kind of object does the function return?
 */
function f($p1, $p2) {
    [...]
}

[nodiscc]

This one has become low priority for me (less time, reprioritizing issues). Anyone is free to work on it and submit a pull request (even small, partial fixes accepted)

[virtualtam]

Yep, the PHPDoc checker is quite annoying and expects comments all over the place :D

Given how the project has evolved since this issue was raised (3 years already!), I think code beautifiers could be run quite safely on all code that has unitary tests, so we can add PSR-2 checks to the CI pipeline.

[ArthurHoaro]

I've starting to work toward this. However note that PSR2 requires us to add a namespace to all classes, which should have be done at some point anyway. But it's a breaking change for 3rd parties. So it should be included in a new major.

Could we include this in the v0.10.0 milestone? Because maintaining such a huge diff until v0.11 will be hard and time consuming.

[nodiscc]

• Reminder to update https://shaarli.readthedocs.io/en/master/Static-analysis/ as this issue progresses