current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite
1

I have a byte array in a C# program.

I want to determine as quickly as possible if the content is Xml. If it's the case, I also want to return the Xml itself.

By now I have this method :

    protected bool TryGetXElement(byte[] body, out XElement el)
    {
        el = null;
        // if there is no data, this is not xml :)
        if (body == null || body.Length == 0)
        {
            return false;
        }

        try
        {
            // Load the data into a memory stream
            using (var ms = new MemoryStream(body))
            {
                using (var sr = new StreamReader(ms))
                {
                    // if the first character is not <, this can't be xml
                    var firstChar = (char)sr.Peek();
                    if (firstChar != '<')
                    {
                        return false;
                    }
                    else
                    {
                        // ultimately, we try to parse the XML
                        el = XElement.Load(sr);
                        return true;
                    }
                }
            }
        }
        catch
        {
            // if it failed, we suppose this is not XML
            return false;
        }
    }

Is there potential improvement?

share|improve this question
    
Why do you need to do this? Can't you figure out whether it's XML some other way? –  svick Aug 27 '12 at 13:27
1  
The bytes are coming from a message streams (that I cannot control). I have to route the messages to either of my parsers, depending of the kind of data. Messages are sometimes binaries, sometimes xml. –  Steve B Aug 27 '12 at 13:35
1  
Does the structure of the messages you are receiving have any metadata that could clue you in as to their contents (e.g., a MIME header)? –  Dan Lyons Aug 27 '12 at 18:37
    
Is there the possibility of having the xml start with a space? –  miniBill Sep 12 '12 at 8:08
    
@minibill: the XML standard states it must starts with a <. Moreover, as it's computer XML, i'm confident it wiki never starts with a sppace –  Steve B Sep 12 '12 at 8:56

2 Answers 2

up vote 1 down vote

Why not verify if a root node exists rather than check for a '<' ?

    public bool GetRootNode(XmlReader reader)
    {
        bool isValid;
        try
        {
            while (reader.Read())
            {
                if (reader.NodeType == XmlNodeType.Element)
                {
                    isValid = true;
                    break;
                }
            }
        }
        catch (XmlException x)
        {
            throw new XmlException(x.Message);
        }

        return isValid;
    }

You could also use the chain of responsibilities pattern to determine which parser you want to use.

EDIT

Not sure if this will work, but here is an example on using the above code:

protected bool TryGetXElement(byte[] body, out XElement el)
{
    el = null;
    // if there is no data, this is not xml :)
    if (body == null || body.Length == 0)
    {
        return false;
    }

    try
    {
        // Load the data into a memory stream
        using (var ms = new MemoryStream(body))
        {
            using (var sr = new StreamReader(ms))
            {
                XmlReaderSettings settings = XmlReaderSettings { CheckCharacters = true; };
                using(XmlReader reader = XmlReader.Create(ms, settings))
                {
                    if (!GetRootNode(reader))
                    {
                        return false;
                    }
                    else
                    {
                        // ultimately, we try to parse the XML
                        el = XElement.Load(sr);
                        return true;
                    }
                }
            }

        }
    }
    catch
    {
        // if it failed, we suppose this is not XML
        return false;
    }
}
share|improve this answer
    
I cannot see why this would be an enhancement. Your code rely on exception catching (so I do ultimately), while mine check one char. I believe checking a single char is very cheap compared to catching an exception, and can reduce a great number of cases (the probability of having a < char in a non xml string is very low). –  Steve B Aug 27 '12 at 16:37
    
You could have a '<' in a string of html. That is not xml. Does your xml have to fit a particular schema? How are you handling invalid characters that cannot appear in an xml document? Do you have to handle those cases? –  NexAddo Aug 27 '12 at 16:43
    
I don't have schema in the sense of XSD files. I have behind the scene a bunch of "parsers" that will analyse the returned XElement. Each parser can handle a well known xml structure. And I also have parsers for non-xml document, but it's another story –  Steve B Aug 27 '12 at 16:47
up vote 1 down vote

Just a few small readability notes:

  1. (body == null || body.Length == 0) could be String.IsNullOrEmpty(body)

  2. Longer variable names than sr, el and ms could be a little bit easier to read. "Without proper names, we are constantly decoding and reconstructing the information that should be apparent from reading the code alone." (From codesparkle's former answer.)

  3. The innermost else could be omitted:

    if (firstChar != '<')
{
    return false;
}

// ultimately, we try to parse the XML
el = XElement.Load(sr);
return true;
share|improve this answer
    
1. no.... body is a byte array not a string. 2. I agree with you at some point. I think I should rename el to foundXElement or something like this. Other very local variables like ms or sr can be stay like this. It's a consistent naming convention I use in very small methods. As always, naming convention is subject to developer's feelings. 3. Technically yes, it can be omitted. But I think it's more readable when the two path are visually on the same level. A matter of preference again. Thanks for your feeback anyways. –  Steve B Sep 27 '12 at 7:51
    
@SteveB: 1: Thanks, I've deleted it. –  palacsint Sep 27 '12 at 7:56

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.