Note draft / work in progress

Rule violations:

  • that are critical or blocker generally indicate bugs and should be fixed as soon possible.
  • that are major may point to areas where issues can occur and care should be taken. You may want to fix these.
  • that are minor can be generally ignored but you may want to comply with them when writing new code.

Fixed here means fixed if there is a bug or marked as won't fix or false positive in the Sonar Cube web interface if it isn't bug.

Note rules that are not commonly encountered, don't occur in the current code base or are minor are not mentioned here.

The resulting rule changes will result in a more relaxed set of rules that match the current development code style of the FlexJS framework and that make the Sonar Cube reports more useful.

For a list of all rules, code example and rule references see here. Click on each rule to see more information.

New Rules

These rules (currently disabled) should be added:

  •  Public classes, methods, properties and metadata should be documented with ASDoc (major)
  •  Method visibility should be explicitly declared (minor)

Critical or Blocking Rules

Any class extending the Event class should override Event.clone()

This is often a bug as without the clone (or similar) method events will not bubble correctly. A false positive may be the case when bubbles is not passed into the constructor but that may also be an omission.

Other rules while probably require no discussion as they generally indicate a serious issue or outdated coding style include:

  • Constructors should not dispatch events
  • "with" statements should not be used
  • Cases in a "switch" should not have the same condition
  • Statements, operators and keywords specific to ActionScript 2 should not be used
  • Switch cases should end with an unconditional "break" statement

Move Rules

These critical rues can be moved to major.

  •  "public static" fields should be constant

  •  The "trace" function should not be used

At some point we may want to consider moving the trace rule back and replacing trace statements with logging calls. (This has been discussed on the dev list.)

Major Rules

Current major rules we should keep. Most of these are simple and safe to fix in some cases provide performance and or code size improvements.

  • "for" loop stop conditions should be invariant
    As to most rules their exceptions, but the most common issue here is checking the length of an array every time around the loop.
  • Collapsible "if" statements should be merged
  • Constructor bodies should be as lightweight as possible
    Code in AS constructors is not JITed. This only effect AS code not the generated JS code. Is performance on AS side a consideration?
  • Event names should not be hardcoded in event listeners
    This has been discussed on the mailing list.
  • Event types should be defined in metadata tags
  • Local variables should not shadow class fields
  • Objects should not be instantiated inside a loop
    Sometime of course you do want to but in some cases this is an issue and can improve performance.
  • Unused local variables should be removed
  • Unused private fields should be removed
  • Unused private function should be removed
  • Useless "if(true) {...}" and "if(false){...}" blocks should be removed

Move Some Major Rules to Minor

List of current major rules to moved to minor. These currently add a lot of noise  or would require a fair amount of effort to comply with or may come with the risk of adding bugs. Moving them to minor would make it easier to see other major issues listed above.

  • "===" and "!==" should be used instead of "==" and "!="
  • "switch case" clauses should not have too many lines
  • "switch" statements should end with a "default" clause
  • Classes should not be too complex
  • Classes should not have too many methods
  • Control flow statements "if", "for", "while" and "switch" should not be nested too deeply
  • Functions should not be too complex
  • Functions should not contain too many return statements
  • Functions should not have too many parameters
  • Methods should not be empty
  • Nested blocks of code should not be left empty
  • Sections of code should not be "commented out"
  • The element type of an array field should be specified
  • The special "star" type should not be used
  • Variables of the "Object" type should not be used
  • Unused function parameters should be removed ?????

Move from Minor to Major

Missing semicolons may introduce errors when optimising and minimising JS code so the missing semicolons rule will move for minor to major. This are possibly other ways to get around this issue but the change is easy and the risk of introducing bugs very low.



 


  • No labels

2 Comments

  1. Piotr Zarzycki

    Hi Justin,

    Could you  expand your description for following points ? I don't know what do you mean:

    • Collapsible "if" statements should be merged
    • Local variables should not shadow class fields

    Please put your answer on this page - I don't need it as an comments here.

    Thanks,

    Piotr

  2. Justin Mclean

    Best if you read the rule descriptions I've added a link to them in the docs above. But basically:

    • if (A) { if (B) {...} } is harder to read than if (A && B) {...}
    • If you have a class property and a local variable with the same name it can be unclear which one you are using/modifying or what the intent of the code was. I've already found an fixed a bug due to this exact issue.