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

["Michael Kay mike@xxxxxxxxxxxx" <xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx>]

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



> On 2 Jul 2017, at 08:42, Mukul Gandhi gandhi.mukul@xxxxxxxxx
<mailto:gandhi.mukul@xxxxxxxxx> <xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx
<mailto:xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx>> wrote:
>
> Hello,
>    There was a time, when I was heavily involved with XSLT.
>
> I had created this web site related to XSLT,
http://gandhimukul.tripod.com/xslt/index.html
<http://gandhimukul.tripod.com/xslt/index.html>
>
> This hasn't been updated in a while.
>
> Looking at this site, can anyone suggest if this site is still valuable? I
can start adding information to this, containing information related to XSLT
3.0 since its now a W3C REC.
>
>
> --
> Regards,
> Mukul Gandhi
> XSL-List info and archive <http://www.mulberrytech.com/xsl/xsl-list>
> EasyUnsubscribe <-list/293509> (by email <>)