Re: [xsl] XSLT site (code quality tool)

Subject: Re: [xsl] XSLT site (code quality tool)
From: "Mukul Gandhi gandhi.mukul@xxxxxxxxx" <xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx>
Date: Thu, 6 Jul 2017 04:58:40 -0000
One point, which I forgot to mention. This tool has a provision to set
desired priority to the rules.

On the site, I've mentioned "I suggest the rule priorities from 1 to 5,
where 1 is a most severe violation and 5 is the least severe".

This could be useful to customize the rules provided in the tool.

On 3 July 2017 at 20:23, Michael Kay mike@xxxxxxxxxxxx <
xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx> wrote:

> Some comments on Mukul's rules (looking both at the prose description and
> the XPath implementation):
> The following rules are supported out of the box, with this tool:
> 1. *DontUseDoubleSlashOperatorNearRoot*: Avoid using the operator // near
> the root of a large tree.
> The implementation checks for "//" at the start of a @select or @match
> attribute (but not @test). Note that with Saxon (I can't speak for other
> implementations), there is no penalty in using //x at the start of a path
> expression (where x is a fully-qualified element name), in fact there may
> be an advantage. However, "//" at the start of a match pattern is a code
> smell.
> And it's interesting that the test expression violates the rule it is
> imposing...
> 2. *DontUseDoubleSlashOperator*: Avoid using the operator // in XPath
> expressions.
> The test for this is //(@match | @select)[not(starts-with(., '//')) and
> contains(., '//')] - i.e. "//" somewhere other than the start.
> Well, it might sometimes be bad to do this, but I wouldn't want to say
> that it's always bad. (Especially not, as in this rule, when the "//" is
> within quotes!) "//" can be legitimate if the intermediate content isn't
> predictable, if the structure is recursive, or if the path is so verbose as
> to become unreadable and error prone.
> Two or more occurrences of "//" within a path expression is definitely
> suspect (but perhaps not in different branches of a union?)
> 3. *SettingValueOfVariableIncorrectly*: Assign value to a variable using
> the 'select' syntax if assigning a string value.
> //xsl:variable[(count(*) = 1) and (count(xsl:value-of) = 1)]
> Can't dispute that one. I would add xsl:param and xsl:with-param. And if
> you're pendantic, it's not "assignment", it's "binding"
> 4. *EmptyContentInInstructions*: Don't use empty content for instructions
> like 'xsl:for-each' 'xsl:if' 'xsl:when' etc.
> You could add a few more instructions to the list. But I think an empty
> xsl:when can be perfectly legitimate.
> 5. *DontUseNodeSetExtension*: Don't use node-set extension function if
> using XSLT 2.0.
> /xsl:stylesheet[@version = '2.0']//@select[contains(., ':node-set')]
> Could improve the test: root element can be xsl:transform, @version can be
> 3.0, attribute can be @test.
> 6. *RedundantNamespaceDeclarations*: There are redundant namespace
> declarations in the xsl:stylesheet element.
> Fair enough: except that using a boilerplate xsl:stylesheet that
> predeclares things like the math namespace isn't actively bad, it can
> certainly be justified.
> The rule looks at xsl:stylesheet but not xsl:transform; and it looks only
> at the start of a @select expression, not anywhere within it. And it
> doesn't look in (e.g) @test attributes or attribute value templates.
> 7. *UnusedFunction*: Stylesheet functions are unused.
> Doesn't detect if the function is used in a different stylesheet module
> (or in an @test attribute or an attribute value template)
> 8. *UnusedNamedTemplate*: Named templates in stylesheet are unused.
> Ditto.
> 9. *UnusedVariable*: Variable is unused in the stylesheet.
> Ditto; but for local variables, the test could be more precise by looking
> only in the following-sibling instructions.
> 10. *UnusedFunctionTemplateParameter*: Function or template parameter is
> unused in the function/template body.
> Looks OK.
> 11. *TooManySmallTemplates*: Too many low granular templates in the
> stylesheet (10 or more).
> Can't see why this is bad. I sometimes have dozens of template rules of
> the form
> <xsl:template match="x[.='ABC']">alphabetic banana
> corporation</xsl:template>
> 12. *MonolithicDesign*: Using a single template/function in the
> stylesheet. You can modularize the code.
> I would say this is only bad if the template is large and contains control
> logic (if/for-each/choose). And if it's the only stylesheet module in the
> stylesheet.
> 13. *OutputMethodXml*: Using the output method 'xml' when generating HTML
> code.
> OK.
> 14. *NotUsingSchemaTypes*: The stylesheet is not using any of the
> built-in Schema types (xs:string etc.), when working in XSLT 2.0 mode.
> Could be a bit stricter: encourage an "as" attribute on every xsl:param,
> for example.
> 15. *UsingNameOrLocalNameFunction*: Using name() function when
> local-name() could be appropriate (and vice-versa).
> Seems too strong. Displaying name() or local-name() is fine; the code
> smell is testing name() for equality with a string literal.
> 16. *FunctionTemplateComplexity*: The function or template's
> size/complexity is high. There is need for refactoring the code.
> The measure is of size rather than complexity. Sometimes a template has to
> include a lot of boring simple code to output a lot of boring simple XML...
> Long and boring doesn't equal complex.
> 17. *NullOutputFromStylesheet*: The stylesheet is not generating any
> useful output. Please relook at the stylesheet logic.
> This seems unlikely to ever fire. Why not test that EVERY
> template/function produces non-trivial output?
> 18. *UsingNamespaceAxis*: Using the deprecated namespace axis, when
> working in XSLT 2.0 mode.
> Deprecated by some. I personally find the namespace axis unobjectionable.
> 19. *CanUseAbbreviatedAxisSpecifier*:  Using the lengthy axis specifiers
> like child::, attribute:: or parent::node().
> I often use unabbreviated axes for clarity, e.g. if (child::*) then ...
> 20. *UsingDisableOutputEscaping*: Have set the disable-output-escaping
> attribute to 'yes'. Please relook at the stylesheet logic.
> No complaints about that one!
> 21. *NotCreatingElementCorrectly*: Creating an element node using the
> xsl:element instruction when could have been possible directly.
> The intent is fine, but I would test (normalize-space(@name) castable as
> xs:NCName)
> 22. *AreYouConfusingVariableAndNode*: You might be confusing a variable
> reference with a node reference. (contributed by, *Alain Benedetti*)
> It's a common mistake but I imagine the rule would give a lot of false
> positives. Why is it only looking at the start of
> xsl:apply-templates/@select? The starts-with() test is too crude. Should do
> a rough tokenization of the XPath expression and look for tokens that are
> equal to the variable name but not preceded by "$".
> 23. *IncorrectUseOfBooleanConstants*: Incorrectly using the boolean
> constants as 'true' or 'false'. (contributed by, *Tony Lavinio*)
> Again, the test would be better if it did a rough tokenization.
> 24. *ShortNames*: Using a single character name for
> variable/function/template. Use meaningful names for these features.
> I think that's paternalistic. There are cases where single-character names
> are entirely appropriate. For example the spec uses N8 for an angle.
> 25. *NameStartsWithNumeric*: The variable/function/template name starts
> with a numeric character.
> That's illegal, so it doesn't need to be included here.
> Some other rules I might add:
> * Using xsl:attribute with content rather than select="" in 2.0.
> * Using translate() for upper/lower case conversion in 2.0.
> * Using xsl:for-each or xsl:template with an xsl:if or xsl:choose as its
> only child
> * Using <xsl:for-each select="empl"><xsl:value-of select="empl"/>
> (expression within for-each is a substring of the expression in the
> containing for-each).
> * Using an expression starting with "/" within an xsl:for-each, unless it
> contains a predicate containing a variable reference or current(). (or
> unless the for-each calls doc() or collection()....)
> * Bad indentation (indentation of an element should be the same as its
> siblings and greater than its parent; indentation defined as the number of
> spaces (tabs??) after the last newline in the preceding whitespace text
> node.
> It would be interesting to rephrase these as rules applied to Saxon's SEF
> export files - you then get the benefit of looking at expressions
> post-parsing.
> Michael Kay
> Saxonica

Mukul Gandhi

Current Thread