Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created an XML wrapper to easily access XML data. Please tell me what do you think about it.

  • Performance
  • Scalability
  • Anything else...

This is how you use it:

var xml = new Xml(dataString);
xml.load("UserEmail");
alert(xml.length + ", " + xml.getValueAt(0)); // Out: 2, [email protected]

XML source file:

<Users>
    <Users>
        <UserEmail>[email protected]</UserEmail>
        <UserPassword>
            BA56E5E0366D003E98EA1C7F04ABF8FCB3753889
        </UserPassword>
    </Users>
    <Users>
        <UserEmail>[email protected]</UserEmail>
        <UserPassword>
            07B7F3EE06F278DB966BE960E7CBBD103DF30CA6
        </UserPassword>
    </Users>
</Users>

Source:

function Xml(xmlString) {
    var parser = function() {
        if (typeof window.DOMParser != "undefined") {
            return (new window.DOMParser()).parseFromString(xmlString,
                    "text/xml");
        }
        else if (typeof window.ActiveXObject != "undefined"
                && new window.ActiveXObject("Microsoft.XMLDOM")) {
            var xmlDoc = new window.ActiveXObject("Microsoft.XMLDOM");
            xmlDoc.async = "false";
            xmlDoc.loadXML(xmlString);
            return xmlDoc;
        }
        else {
            throw new Error("XML parser not found");
        }
    };

    var data = parser(xmlString);
    var elements = null;
    this.length = 0;

    this.load = function(nodeName){
        elements = data.documentElement.getElementsByTagName(nodeName);
        this.length = elements.length;
    };

    this.getValueAt = function(index) {
        if(!elements || index >= this.length){
            return null;
        }
        var element = elements.item(index);

        return element.childNodes[0].data;
    };
}
share|improve this question

migrated from stackoverflow.com Jun 24 '12 at 16:28

This question came from our site for professional and enthusiast programmers.

    
It can usefully handle very simple XMLs. It doesn't retrieve the value of attributes. It gives worthless values for element nodes. It doesn't support a decent selector engine. If it's good for your purposes, use it. –  MaxArt Jun 23 '12 at 12:59
    
"Would you write your own XML Parser? Only if you're f***ing crazy." secretgeek.net/csv_trouble.asp –  David East Jun 23 '12 at 13:33
    
@David pmpl nice one ;) –  Babibu Jun 23 '12 at 13:40

1 Answer 1

up vote 2 down vote accepted

From a quick read :

  • Xml seems like a bad name for your wrapper, you should consider something like xmlParser ?

  • I would allow access to data and elements by using this instead of var because you wrap so little of the XML parser API

  • this.length seems wrong ( the parser has no length), maybe loadedElementCount, but even that is pretty bad, I would just let the caller use elements.length.

  • I would return elements in this.load, since that is pretty much what the caller would need next.

  • You are not checking for falsey values of nodeName in this.load

  • I would not create a var element in getValueAt, I would just return elements.item(index).childNodes[0].data

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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