4
\$\begingroup\$

Source XML (since I haven't bothered with a schema):

<?xml version="1.0"?>
<ContactDetails>
  <Names>
    <FullName>
   Nicholas Example
  </FullName>
  <AltName context="Nickname">
    Nick
  </AltName>
  <AltName context="Online">
   oxinabox
  </AltName>
  </Names>
  <Phones>
    <Phone context="Mobile">04 1234 1234</Phone>
  </Phones>
  <Addresses>
    <Address context="Semester">
      Contrived Apartments
      <number> 5 </number> <street>Some Rd</street>
      <town>Newtown</town> <state>ST</state>
    </Address>
  </Addresses>
  <Emails>
    <Email context="Work">examplen@work.com</Email>
    <Email context="Personal">nick99@ManMail.com</Email>
  </Emails>
  <Websites>
    <Website>http://www.a2b3c4.com</Website>
  </Websites>

  <Banking>
   <BankAccount context="Everyday" 
      Name="Nicholas Example" 
      BSB="123 123" 
      AccountNumber="111 222 333" />

  </Banking>
</ContactDetails>

and here is the actual XSLT I would like a review of. It is supposed to turn the XML in to a human readable flat file (and does except for addresses which I am still working on):

<?xml version="1.0" ?>
<xsl:stylesheet version="1.0" 
    xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
    xmlns:fn="http://www.w3.org/2005/xpath-functions"
    >
    <xsl:output
        method="text"
        omit-xml-declaration="yes"
        indent="no"
        media-type="text/plain"/> 
  <xsl:strip-space elements="*"/>


    <xsl:template match="ContactDetails">
        <xsl:apply-templates/>
    </xsl:template>

    <xsl:template match="Names">
    <xsl:value-of select="FullName"/>
    <xsl:for-each select="AltName"> 
            <xsl:text> AKA </xsl:text><xsl:value-of select="normalize-space(.)"/>
        </xsl:for-each>
        <xsl:text>
        </xsl:text>
    </xsl:template>

    <xsl:template match="//Phone">
      <xsl:text>
Ph (</xsl:text><xsl:value-of select="@context"/> <xsl:text>): </xsl:text>
        <xsl:value-of select = "."/>
    </xsl:template> 

    <xsl:template match="//Email">
      <xsl:text>
Email (</xsl:text><xsl:value-of select="@context"/> <xsl:text>): </xsl:text>
        <xsl:value-of select = "."/>
    </xsl:template> 

    <xsl:template match="//Website">
      <xsl:text>
Website: </xsl:text>
        <xsl:value-of select = "."/>
    </xsl:template> 

    <xsl:template match="//Address">
<!--        <xsl:value-of select ="."/>
        <xsl:for-each select="*">
        <xsl:value-of select = "."/>
                <xsl:if test="fn:name(.)=town">
                    <xsl:text>, </xsl:text>
                </xsl:if>
        </xsl:for-each>
        -->
    </xsl:template> 



    <xsl:template match="Banking">
<xsl:text>

Bank Details:
------------
</xsl:text> 
        <xsl:for-each select="BankAccount">
            <xsl:value-of select="@context"/> <xsl:text> Account: </xsl:text>
            <xsl:value-of select="@BSB"/>-<xsl:value-of select="@AccountNumber"/>
        </xsl:for-each>
    </xsl:template>

</xsl:stylesheet>
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I tend to like email addresses in First Last <name@place.domain> format. It lets me easily select just the name. And if I select the whole thing their name shows up in the outgoing email and my mail software will remember them by their name. \$\endgroup\$ – Omnifarious Mar 18 '12 at 15:45
3
\$\begingroup\$
<xsl:output
    method="text"
    omit-xml-declaration="yes"
    indent="no"
    media-type="text/plain"/> 

Consider using XSLT 2.0, which can't hurt but could help if you need a XSLT 2.0 function in the future. Also note that you don't need omit-output-declaration since you're outputing text. Ditto for indent.

<xsl:template match="ContactDetails">
    <xsl:apply-templates/>
</xsl:template>

This is the default template for every element, which means it's not needed.

<xsl:template match="//Email">

Why the //? It makes more sense without it, eg. match="Email". This applies to all your templates.

    <xsl:text>
Ph (</xsl:text><xsl:value-of select="@context"/> <xsl:text>): </xsl:text>
    <xsl:value-of select = "."/>

You're using xsl:text too much. This could become:

Ph (<xsl:value-of select="@context"/>): <xsl:value-of select = "."/>

Besides, I'm not sure context is a good choice for an attribute name. "Context node" already has a specific meaning in XSLT.

\$\endgroup\$
4
  • \$\begingroup\$ When shound/shouldn't I use xsl:text? \$\endgroup\$ – Lyndon White Mar 18 '12 at 23:40
  • \$\begingroup\$ I personally only use it to introduce \n to my output. \$\endgroup\$ – Quentin Pradet Mar 19 '12 at 7:19
  • \$\begingroup\$ It seems ifiremoveany of the xsl:text tags I start seeing strange new lines next to every piece of text I put in \$\endgroup\$ – Lyndon White Mar 19 '12 at 11:51
  • \$\begingroup\$ XSLT 2.0 engines are is so incredibly rare. To my knowledge the only freeone is Saxon. \$\endgroup\$ – Lyndon White Feb 10 '15 at 14:47
3
\$\begingroup\$

you should really work on the indentation of your code, there is nothing worse than trying to read code that isn't properly indented.

Your indentation

<xsl:template match="Names">
<xsl:value-of select="FullName"/>
<xsl:for-each select="AltName"> 
        <xsl:text> AKA </xsl:text><xsl:value-of select="normalize-space(.)"/>
    </xsl:for-each>
    <xsl:text>
    </xsl:text>
</xsl:template>

What it should look like

<xsl:template match="Names">
    <xsl:value-of select="FullName"/>
    <xsl:for-each select="AltName"> 
        <xsl:text> AKA </xsl:text>
        <xsl:value-of select="normalize-space(.)"/>
    </xsl:for-each>
    <xsl:text>
    </xsl:text>
</xsl:template>

Again, Yours

<xsl:template match="//Phone">
  <xsl:text>
Ph (</xsl:text><xsl:value-of select="@context"/> <xsl:text>): </xsl:text>
    <xsl:value-of select = "."/>
</xsl:template> 

Proper

<xsl:template match="//Phone">
  <xsl:text>Ph (</xsl:text>
  <xsl:value-of select="@context"/> 
  <xsl:text>): </xsl:text>
  <xsl:value-of select = "."/>
</xsl:template> 

there is nothing worse than having to stop and think to see what is nested inside of what. some people have a different style of indenting but what you have posted looks very confusing.

another proper way to write the Phone Template

<xsl:template match="//Phone">
    <xsl:text>
        Ph (
    </xsl:text>
    <xsl:value-of select="@context"/> 
    <xsl:text>
        ): 
    </xsl:text>
    <xsl:value-of select = "."/>
</xsl:template> 

I know this was nitpicky but it helps to keep your code organized in a similar manner to other people that will be reviewing your code, debugging your code, or adding to your code, so that they can understand what is going on quickly.

(most of this you probably already know and just had a fun time trying to get your code to paste right into the Code Review Question box)

\$\endgroup\$
3
  • \$\begingroup\$ Actually, my indenting is that way, because I have constant worries and issues with ensuring that my output in not indented/ line broken in the wrong place. Especially since the output format is a flat file, and so I can not rely on xhtml to only acknowledged <br> and to remove an needed whitespace. It may be my paranoia was for nothing. \$\endgroup\$ – Lyndon White Nov 17 '13 at 2:21
  • \$\begingroup\$ @Oxinabox, have you tried it this way? don't use the last example. I could see how that last example would add newlines in your flat file. but the other "proper" examples shouldn't do that. because the static text is inside of text tags \$\endgroup\$ – Malachi Nov 17 '13 at 5:22
  • \$\begingroup\$ I don't know this was over 18 months ago \$\endgroup\$ – Lyndon White Nov 17 '13 at 7:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.