Re: XSLT site (code quality tool)

Hi Mike,
   Thanks for a nice analysis, of this tool. If time permits me, I'll
modify this tool as per your suggestions.

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
>
>
>
> On 2 Jul 2017, at 08:42, Mukul Gandhi gandhi.mukul@xxxxxxxxx <
> 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
>
> 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