xsl-list
[Top] [All Lists]

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

2017-07-05 23:58:48
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(_at_)saxonica(_dot_)com <
xsl-list-service(_at_)lists(_dot_)mulberrytech(_dot_)com> 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 θ 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





-- 
Regards,
Mukul Gandhi
--~----------------------------------------------------------------
XSL-List info and archive: http://www.mulberrytech.com/xsl/xsl-list
EasyUnsubscribe: http://lists.mulberrytech.com/unsub/xsl-list/1167547
or by email: xsl-list-unsub(_at_)lists(_dot_)mulberrytech(_dot_)com
--~--
<Prev in Thread] Current Thread [Next in Thread>