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 > > > -- Regards, Mukul Gandhi
Current Thread |
---|
|
<- Previous | Index | Next -> |
---|---|---|
Re: [xsl] XSLT site (code quality t, Mukul Gandhi gandhi. | Thread | [no subject], Unknown |
Re: [xsl] XSLT site (code quality t, Mukul Gandhi gandhi. | Date | [xsl] Xpath conditional, Joseph L. Casale jca |
Month |