jenkins has many complaints about PHP code

[cgolubi1]

Current output of a jenkins run on buttonmen-dev master includes:

• 2198 "checkstyle" warnings
• 160 "PMD" warnings
• 8 "duplicate code" warnings
• 77% PHP code test coverage
• 606 "API documentation" errors (i think these are all duplicates with checkstyle complaints about the same)

Obviously, we can't really enforce coding standards using this tool under these conditions. Look at these and figure out which ones are real errors (and fix those) and which ones are a coding standard we like but Jenkins doesn't (and fix jenkins).

[cgolubi1]

I decided that all the "duplicate code" warnings were legitimate --- three things that i'd looked at previously and said, "ugh, i should fix this", and one of James's which was similar. I fixed those all, and verified that the "duplicated code warnings" sidebar disappears from the page when there are no duplicated code warnings, which is nice from the point of view of making it easy to find things.

[cgolubi1]

Okay, what about the things that PMD complains about?

• 58 of its complaints are "Code Size" rules - i'm not going to make a call on any of these without James's opinion:
  • CyclomaticComplexity: 24
  • ExcessiveClassComplexity: 4
  • ExcessiveClassLength: 2
  • ExcessiveMethodLength: 10
  • NPathComplexity: 13
  • TooManyFields: 1
  • TooManyMethods: 4
• 61 are "Naming Rules":
  • ConstantNamingConventions: 13 (all in BMGameState) - it thinks constants should be named in allcaps. Is it right? Probably?
  • LongVariable: 40 - it thinks variables should have names under 20 characters. Let's discuss - i don't feel strongly about this.
  • ShortVariable: 8 - it thinks important variables should have names of at least 3 characters. I agree with it, and think James does too; almost all of the code in violation appears to be quite old, so i think we could fix it all and follow this rule without too much stress.
• 38 are "Unused Code Rules":
  • UnusedFormalParameter: 16 - complicated. The ones it's complaining about in BMAttack are because the parent class defines these functions which will always fail, and child classes are supposed to override them. Maybe we're doing that wrong? Some of the other ones it found (e.g. in BMAttackSkill) i think really are typoes.
  • UnusedLocalVariables: 15 - it has some weird ideas about what's going on here. I'm not at all sure if it's wrong or we are.
  • UnusedPrivateField: 7 - i think it's right about all of these. They're fields that were initially defined as BMGame properties, but are now automatically derived on the fly when anyone asks for them. Therefore, we shouldn't have them be properties.

Summarizing PMD complaints:

• Categories where my tentative opinion is that Jenkins is right, and we should probably fix what it's complaining about:
  • ConstantNamingConventions
  • ShortVariable
  • UnusedPrivateField
• Categories where my tentative opinion is that we should change the check:
  • LongVariable - maybe make the threshold 30 instead of 20, so we have some limit while being compatible with James's coding style (which isn't really my coding style, but i'm okay with it)
• Categories where we need to discuss further:
  • all code size rule types
  • UnusedFormalParameter
  • UnusedLocalVariable

[cgolubi1]

How about checkstyle? There are a lot of categories here, and we're clearly going to have to customize. Let's start with the right baseline. Here are the formats installed with Jenkins by default. Right now, these give me:

• PEAR: 2164 warnings
• PHPCS: 6834 warnings - i didn't realise that this is a strict superset of PEAR. We don't want that.
• Squiz: 6366 warnings
• MySource: a project-specific (not our project) modification of Squiz, skip it
• Zend: claims to be out of date
• PSR1: 213 warnings (wow)
• PSR2: 1220 warnings (hmm)

So, of all of those, i think i like the idea of starting with PSR2 and changing some stuff. So, step 1 is to figure out how to do that, which was pretty easy --- we now have a custom ruleset we're committing, which happens to follow the same rules as PSR2 right now. Cool. So, things where i can see that it's right, i'm fixing as i find them.

• BraceOnSameLine: that's no good -- we want our braces on the same line. I did some futzing around online and figured out how to make this customization to the style, and it seems to have worked.
• MissingNamespace: each PHP file should have a namespace. My lack of desire to revisit this is boundless. I've excluded it.

Here are the highest-hitcount things where i'm not sure what to do without talking to James:

Found: it expects "true", "false", and "null" to be lowercase. Well, that's what i do most of the time (though i'm not consistent), so i'm fine with it. James, what do you want to do?

CloseBracketLine: if you have a multi-line function call, it wants the closing paren to be on a line by itself, e.g. it's complaining about:

    setcookie(session_name(), '', time()-3600,
              $params["path"], $params["domain"],
              $params["secure"], $params["httponly"]);

and many others. I'm fine with this either way --- i'm happy to do what it wants here, or i'm happy to try to change it if we understand what we'd like better.

NotCamelCaps: it thinks all our class and method names should be camelcase. I think we're intentionally not doing that, but i can't necessarily articulate what our class/method name standard is.

Indent: it doesn't like

    setcookie(session_name(), '', time()-3600,
              $params["path"], $params["domain"],
              $params["secure"], $params["httponly"]);

It wants us to instead do:

    setcookie(session_name(), '', time()-3600,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]);

(i.e. four indents, not indenting to the open brace). I used the same example as i did for CloseBracketLine because i think these are in fact related, in that i'd be pretty happy with:

    setcookie(
        session_name(), '', time()-3600,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]
    );

which i think would resolve both complaints.

Ah, that change would also resolve ContentAfterOpenBracket. No points for guessing what that one wants. :>)

Ah, one more --- it really wants us to do:

    setcookie(
        session_name(),
        '',
        time()-3600,
        $params["path"],
        $params["domain"],
        $params["secure"],
        $params["httponly"]
    );

and that one would resolve MultipleArguments. I'm fine with all that if you are, and can suppress any of them instead if you're not.

NotAllowed: it doesn't like "else if" (fine, we usually use "elseif", so i fixed the couple of errors), but more importantly, it thinks PHP files may not end in ?>. Hmm. What do we know?

ControlStructures,ContentBefore: it doesn't like a brace followed by more code on the same line, e.g.:

        if (count($helpers) == 0) { return array($helpMin, $helpMax); }

I don't really like it either, and think we should fix those. They look like old code anyway. But i want to double-check with you first.

TooLong: PSR2 complains at 120 characters. I like that --- i think it's a good idea not to have arbitrarily-long lines, and in fact very few of our lines are not in compliance with 120 characters. I fixed a bunch in dummy_responder that i'd just been too lazy to fix ealier, and will double-check with you before addressing the BMGame ones.

OneParamPerLine,ContentAfterComma,FirstParamSpacing: this is for multi-line function declarations (not invocations) and only affects BMInterface --- it wants us to put one parameter per line for a multi-line function declaration, and to put the first parameter on its own line, not with the function. What do you think?

FoundWithSymbols: You're going to have to translate these for me --- it's complaining about any file containing both class declarations and other code, and sees this in dummy_responder, responder, BMInterface, and BMDie. Let's discuss.

Whew. Okay, well, that was a long exercise, but at the end of it, we're down under 1000 checkstyle errors, and i think i understand the conversations we need to have to decide what to do about almost all of them.

[cgolubi1]

Notes from discussion with James:

• Hard line limit of 120 chars is fine with both of us
• He likes uppercase TRUE/FALSE/NULL, i'll figure out how to make checkstyle behave
• We should adhere to the naming capitalization stuff from https://github.com/buttonmen-dev/buttonmen/wiki/Developer-guide%3A-coding-style, so i need to figure out how to make checkstyle do that
• For files containing only PHP code (which is all of our .php files), we will omit the closing ?>
• ControlStructures,ContentBefore: yep, fix these
• OneParamPerLine et al: we don't care about one param per line, but i (Chaos) would rather reduce some horizontal whitespace for these. We'll try taking all the parameters off the first line and reducing the indentation, and see how that goes
• FoundWithSymbols: we'll leave these for the moment. We have some ideas about its complaint --- in src/engine, the requires, which may be vestigial; in src/api, the fact that the files contain both classes and runnable code --- but we're not going to deal with it right now.
• Game state constants: it's fine to make these all-caps with underscores, as long as we fix them everywhere they appear

Stuff in PMD:

• complexity: i'm going to try refactoring responder per http://codepad.org/vSVQopwL, which we think may wind up resolving several of these. James will see how it looks and whether he'd like to adopt it for BMGame etc. Other than that, we won't worry about these yet, and we expect that some of them (class length, too many fields, too many methods), we'll eventually move to suppress
• LongVariable: we'll defer for now
• ShortVariable: we should fix these
• Unused*: we should fix these

[cgolubi1]

Okay, i did a number of fixes. In fact, i opted to go ahead and almost fully fix checkstyle. Six errors remain, which i think we can probably defer/ignore for some time:

• FoundWithSymbols (4): as noted, we're deferring this
• NotCamelCaps (2): i told it not to enforce CamelCaps methods, but it's doing the right thing for most of our class names, and just having trouble with responder and dummy_responder, which are a weird case anyway (and are kind of responsible for two of the FoundWithSymbols errors)

Almost all of the PMD errors still remain --- James, could you take care of ShortVariable, UnusedFormalParameter, UnusedLocalVariable, and UnusedPrivateField? I'm still on the hook to spec out a cyclomatic complexity improvement.

I'm going to go ahead and put in a pull request with what i've got right now, because i think it's a significant improvement and also it touches a lot of code, so i'd like to get it in the queue before too many other things change.

[blackshadowshade]

UnusedPrivateField warnings cannot be removed. This is a problem that has arisen from PHP encapsulation and enforcement of getters and setters. Our private fields are actually almost all accessible as if they were public, only they have defined get and set behaviour.

[blackshadowshade]

@cgolubi1, here are the tweaks that I would like to make to the Jenkins PMD checking:

• increase cyclomatic complexity threshold from 10 to 13
• remove checking of NPathComplexity
• remove TooManyMethods check
• remove ExcessiveClassLength check

[cgolubi1]

Anything magic about 13? That is, if something pops up with a cyclomatic complexity of 14, will we want to debug it, or raise the threshold again? (Either answer is fine, just curious.)

[blackshadowshade]

Actually, yes, there is something magic about 13.

The general guidelines for cyclomatic complexity suggest that 10 is a warning sign, and 15 is an absolute limit.

I tend to use two or three up on input parameter validation, and I don't really consider those to be part of the main function logic.

Thus, we have 10 + 2 allowed, and 13 should starts signalling again.

[blackshadowshade]

Add NumberOfChildren to the PMD warnings that we want to ignore.

[blackshadowshade]

I'll use my 'bug fix of the month' for April on this issue. I know I've mentioned it on IRC and it's already on Chaos' to-do list, but I figure that this is the one issue that is potentially affecting my ability to develop shinier code. :)

[cgolubi1]

Sounds good. (Everyone else still has one more day of March to complete those games, since we're counting this in U.S. time. I'll send e-mail in the morning to the buttonmen-dev list about who is owed bugfixes.)

[cgolubi1]

For future reference: http://phpmd.org/documentation/creating-a-ruleset.html explains how to create a PMD ruleset that imports and customizes existing rulesets.

[cgolubi1]

Okay, i've almost got this (see http://jenkins.buttonweavers.com:8080/job/buttonmen-cgolubi1/208/), and i think my changes correctly addressed:

• increase cyclomatic complexity threshold from 10 to 13
• exclude NPathComplexity
• exclude TooManyMethods
• exclude NumberOfChildren

However, ExcessiveClassLength doesn't actually match any warnings (see http://jenkins.buttonweavers.com:8080/job/buttonmen-cgolubi1/208/pmdResult/fixed/ for the absence of that string). Did you mean to ask me to exclude ExcessiveClassComplexity instead?

[blackshadowshade]

Yeah, that's the one. ExcessiveClassComplexity.

[blackshadowshade]

Removed bugfix-of-the-month label on this because Chaos has done her part.

[cgolubi1]

I'm going to use my bugfix-of-the-month for this, in particular for getting the PMD warning count down to zero.