____Review and audit guidelines

Description

This is a guide for reviewing a customer's Magnolia project sources. It is based on this Project Audit Matrix but is more focused on the code and files of a Magnolia project than the deployment and performance of the project. 

Goal

The point of the review is to give the customer feedback on their Magnolia project. Some of that feedback may be stylistic or opinionated, and the customer may ignore some or all of it. The point of the review identify things that vary from what we consider as best practice, gauge their importance or severity, and recommend fixes or alternative approaches. 

Each review item is classified:

Best practice: Magnolia recommended - organization and approaches corresponding to the Magnolia roadmap and avoidance of common Magnolia problems.

Best practice: clean code - general guidelines for writing clean code (usually Java), not necessarily Magnolia specific but helpful to maintaining your Magnolia project.

Each review item is evaluated by severity or significance: 

Problem: immediate correction recommended - a problem that should be corrected as soon as possible, may cause significant headaches if left as is. 

Problem: correct soon - a problem that should be addressed in the near future, may cause difficulties in using a new Magnolia release or not be in alignment with the Magnolia roadmap.

Problem: minor concern - a stylistic or cosmetic problem that can affect the readability of code

The review covers the following areas: 

1. Magnolia project setup
2. Magnolia module organization
3. Magnolia light development
4. Magnolia themes
5. Magnolia Freemarker templates
6. Magnolia Java code
7. Magnolia legacy
8. Magnolia operation (optional)

Review: Magnolia project setup

<version>[^\$]

Review: Magnolia module organization

BootstrapSingleModuleResource(String name, String description, String resourceName, String subPath).

Review: Magnolia light development

Review: Magnolia themes

Review: Magnolia Freemarker templates

Review: Magnolia Java code

Review: Magnolia legacy

This section should be expanded!

Review: Magnolia operation (optional)

Note: this section is being developed and will be expanded in the future. Customers don't usually provide access to their development, test and production environments so this part of the review may need to be done in discussion with the customer.

Review tools

Checking lots of source files by eyeball is boring and difficult to do consistently and accurately. While not all of the above checks can be automated, some checks be automated. 

You can find all the latest versions of the review scripts mentioned above here: GIT repo: audit-scripts

inccheck.pl - a perl script to count includes in YAML definitions. 

Pass the paths of YAML files to this script and it will report files with excessive numbers of includes. 

Command line options: 

inccheck.pl [-t <include threshold>] [-a] [-v] [-V] [-r] [-p <path prefix] <YAML file...>

-t <include threshold>

Sets the threshold for the number of includes that will be reported, the default is 0, meaning any inclusion will be reported (probably not what you want).

-a 

Reports all inclusions in the input files, overriding the setting of the include threshold (-t). 

-r 

Check for recursive includes (e.g. including a definition that contains inclusions).

-p

Remove the path prefix from the YAML file path. Since the YAML file path may not be the same as the final resource path. Removing the path prefix should be used to transform a file path to the resource path of the YAML file so the check for recursive inclusions (-r) will be correct.

-v

Generate debugging output. 

-V

Generate much more debugging output.

rinccheck.pl - a perl script to check for recursive includes in YAML

Print out an include tree for YAML files. For each file, any included YAML will be listed with the indentation and each included file will show its inclusions, ad nauseum.

Command line options: 

rinccheck.pl [-v] [-V] [-p <path prefix] <YAML file...>

-v

Generate debugging output. 

-V

Generate much more debugging output.

-p <path prefix>

Remove the path specified from the output.

templatecheck.pl - a perl script to check the naming of templates and definitions

Pass the paths of YAML definitions to this script and it will check the naming of definitions of templates, definitions and dialogs.

Will report: 

• if the template definition name is different from the Freemarker template script name
• if the template script in the YAML definition does not exist
• if the dialog definition has been defined
• if the dialog definition does not exist

You can perform individual checks using the -N, -F, -D options or use the -A option to check for everything.

Command line options: 

templatecheck.pl [-A] [-N] [-F] [-D] [-p <template path prefix>] [-d <dialog path prefix>] [-v] <YAML template definitions...>

-A
Check all (dialog existence, Freemarker existence and Freemarker/template definition naming. Note: is equivalent to -N -F -D.

-N

Check if the naming of the Freemarker template matches the naming of the template definition (e.g. mytemplate.yaml → mytemplate.ftl)

-F

Check if the Freemarker template in the template definition exists. 

-D

Check if the dialog is defined in the template definition and the dialog YAML file exists.
-p <template path prefix>

Path prefix used to locate the corresponding Freemarker template for the YAML template definition. 

-d <dialog path prefix>

Path prefix used to locate the corresponding dialog definition for the YAML template definition

-v 

Generate copious debugging output

Example execution

perl ~/audit-scripts/templatecheck.pl -A \
-p ~/magnolia-audits/client/project/light-modules/ \
-d ~/magnolia-audits/client/project/light-modules/global/dialogs/ \
templates/components/landingText/landingText.yaml \
templates/components/articleCallout/articleCallout.yaml \
templates/components/video/video.yaml \
templates/components/card/card.yaml

Note: The template path is the root of the common light module directory. Template script references are literal. ie. /name-of-module/path/within/module

The dialog path assumes all dialogs are in the same module as the template definition. Dialog references use a registry and begin with a module name. ie. name-of-module:path/within/dialogs/folder

ldcheck.pl - a perl script to check for various Light Development best practices, including: 

• use of "renderType: freemarker" properties (not necessary, could use default)
• hardcoded values for label and description properties (should use auto generated I18N keys)
• empty label properties
• label and description values using I18N keys that do not correspond to auto generated I18N keys (auto generated I18N keys preferred)
• label and description values using auto generated I18N keys (unnecessary)

Command line options

ldcheck.pl [-v] <YAML template definitions...>

-v

Generate copious debugging output

bsanalysis.pl - a perl script to check for module bootstrapping 

Checks for modules bootstrapping content into other modules. 

Command line options

bsanalysis.pl [-v] [-s] [-r <project root directory>]<directory>

-v

Generate lotsa debugging output

-s 

Check the bootstrap samples directories as well. If not specified, the bootstrap samples directories will be ignored.

-r 

Specify the project root directory. If not specified, the current working directory will be used.

taskcount.pl - count tasks used in version handlers

Counts each task used in version handlers and prints out a listing of tasks plus how many times they were used.

Command line options

taskcount.pl [-v] [-t] <version handler source file>

-v

Generates debugging output

-t 

Print out the tasks found and their counts across all version handlers supplied.

labelxref.pl - a perl script to cross reference usage of i18n labels in yaml definitions

Searches for "label:" references in input files and outputs the label values and the files it is used in.

Command line options

labelxref.pl -v <yaml definition files>

-v 

Generate beaucoup debugging output.

Automated Java code review tools

There are many automated code review tools out there. I chose PMD: 

https://pmd.github.io

PMD is not necessarily better or worse than any other tool out there, but it is useful because:

• it's open source (versus a commercial product requiring a licence)
• flexible, you can choose which rule sets you use to analyze Java sources
• can be run from the command line

The goal is use the automated code review tools is locate sources that should be inspected by you (the human). 

Automated code review tools can produce tons of output about nit picky details, so it is best to use rules/standards for obvious problems or poor style. 

The following PMD rule sets were the best for finding problems / poor style without generating too output:

• Basic: The Basic ruleset contains a collection of good practices which should be followed.
• Code Size: The Code Size ruleset contains rules that find problems related to code size or complexity.
• Import Statements: These rules deal with different problems that can occur with import statements.
• J2EE: Rules specific to the use of J2EE implementations.
• Security Code Guidelines: These rules check the security guidelines from Sun, published at http://java.sun.com/security/seccodeguide.html#gcg
• Unnecessary: The Unnecessary Ruleset contains a collection of rules for unnecessary code.
• Unused Code: The Unused Code ruleset contains rules that find unused or ineffective code.

NOTE: pmd v5.* included the above rule sets. However, pmd v6.* deprecated these rule sets. You can run them but pmd will complain about using deprecated rules. 

Here is a custom rule set definition that includes all rules from the above rule sets and can be executed without pmd complaints: 

• Bechtle review (XML)
  

Review templates

• Project review template (Pages)
  

An Apple Pages document containing a boilerplate description of the review checks described above.

A good starting point for generating a review document.