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.
private string ExtractExceptionMesssage(string exceptionMessage)
{
    const string startWord = "Message&gt";
    int startWordLength = startWord.Length;
    const string endWord = "/Message&gt";
    var length = exceptionMessage.Length;
    int index = 0;
    var sb = new StringBuilder();

    while (index < length)
    {
        var startIndex = exceptionMessage.IndexOf(startWord, index, StringComparison.Ordinal);
        if (startIndex < 0)
            break;
        var endIndex = exceptionMessage.IndexOf(endWord, startIndex, StringComparison.Ordinal);
        if (endIndex < 0)
        {
            break;
        }
        var len = endIndex - 4 - (startIndex + startWord.Length); // -4 is ".&lt"
        sb.AppendLine(exceptionMessage.Substring(startIndex + startWordLength, len));
        index = endIndex + endWord.Length + 1;
    }

    if (sb.Length == 0) sb.Append("Unknown error");
    return sb.ToString();
}

I'll appreciate any idea of how to make it better

share|improve this question

2 Answers 2

up vote 4 down vote accepted

Consistent style
You kind of mixing your styles with curly braces - your indentation is inconsistent, and for two duplicate ifs with single statement you once omit the braces, and on the other not. Choose a style, and stick to it - it will make your code more readable

Magic numbers
You made a curious choice to omit the "&lt;" from you start and end indicators, which made you add the mysterious - 4 to your len calculation. This is a major maintenance issue, since tomorrow you will want to extract with unescaped text, where the "&lt;" will be reduced to "<", and your code will stop working...

Use the powers of the tools at hand
Your solution could be much simpler if you used Regex to solve it:

private string ExtractExceptionMesssage(string exceptionMessage) {
    string pattern = @"&lt;Message&gt;(.+?)lt;/Message&gt;";
    Regex rgx = new Regex(pattern);
    var sb = new StringBuilder();
    MatchCollection matches = rgx.Matches(input);
    if (matches.Count > 0) {
       foreach (Match match in matches) {
         sb.AppendLine(match.Value);
       }
    } else {
       return "Unknown error";
    }
    return sb.ToString();
}
share|improve this answer

This looks like XML in XML (that's why there are &gt;s instead of >s). When working with XML, you should use a XML parsing library, like LINQ to XML, instead of trying parse it manually.

Assuming you can use LINQ to XML to get the XML unencoded (with >s) and that it's actually valid XML (with a single root element), then you can use something like:

private string ExtractExceptionMesssage(string exceptionMessage)
{
    var doc = XElement.Parse(exceptionMessage);
    return string.Join("\n", doc.Descendants("Message").Select(m => m.Value));
}
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.