Subject: Re: [xsl] XSLT site (code quality tool) From: "Michael Kay mike@xxxxxxxxxxxx" <xsl-list-service@xxxxxxxxxxxxxxxxxxxxxx> Date: Mon, 3 Jul 2017 14:53:09 -0000 |
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 <>)
Current Thread |
---|
|
<- Previous | Index | Next -> |
---|---|---|
Re: [xsl] XSLT site, Alan Painter alan.pa | Thread | Re: [xsl] XSLT site (code quality t, Mukul Gandhi gandhi. |
Re: [xsl] XSLT site, Mukul Gandhi gandhi. | Date | Re: [xsl] XSLT site (code quality t, Mukul Gandhi gandhi. |
Month |