What i'm doing

I have a string with html information like this:

<p> <span class="fieldText" fieldId="field-4">Some text</span> this is a test</p>

My goal in the method is to create a dictionary with this value:

**key**     **value**
field-4    Some text

This is the code that i'm using to accomplish my task:

public static Dictionary<int,String> getFields(String mensaje) 
        Dictionary<int,String> fields = new Dictionary<int,string>();
        Match m = Regex.Match(mensaje, @"^(.*?<span .*?>(.*?)</span>.*?)+$", RegexOptions.Singleline);
        for (int i = 0; i < m.Groups[2].Captures.Count; i++)
            String value = m.Groups[1].Captures[i].Value;
            Match m2 = Regex.Match(value, "^(.*?fieldId=.*?\"(.*?)\">.*?)+$", RegexOptions.Singleline);
            String fieldId = m2.Groups[2].Captures[0].Value;
            fieldId = fieldId.Replace("field-", String.Empty);

        return fields;

How can i improve my code?

  • 4
    \$\begingroup\$ What about your gold? \$\endgroup\$ Jul 19 '11 at 20:38
  • \$\begingroup\$ @Steven Jeuris, hi, what do you mean with about change my gold?? \$\endgroup\$
    – Jorge
    Jul 19 '11 at 21:50
  • \$\begingroup\$ You used the word 'gold' in your question where I think you meant to say 'goal'. Steve is just teasing you a bit =). \$\endgroup\$
    – Nathanael
    Jul 19 '11 at 21:52
  • \$\begingroup\$ "My gold in the method it's a dictionary with this value", I'm guessing you mean something along the lines of "My goal is to extract the values into a dictionary as follows:", but I'm not quite sure. \$\endgroup\$ Jul 19 '11 at 21:53
  • \$\begingroup\$ @Steven Jeuris thank's for you concern I'm reaylly appreciate \$\endgroup\$
    – Jorge
    Jul 19 '11 at 22:05

I know this is Code Review not Rewrite My Code, however I would suggest using a third-party Html parser (like the Html Agility Pack for example) over regular expressions if that's an option.

I realize you're doing very trivial parsing here, but from my personal experiences regular expressions grow to unmaintainable status quicker than anything in software development.

If you were to use a Html parser, you could do something like this:

string htmlToParse = "<p><span class=\"fieldText\" fieldId=\"field-4\">Some text</span> this is a test</p><p><span class=\"fieldText\" fieldId=\"field-5\">Some more text</span> this is another test</p>";
const string ElementToParse = "span";
const string IdField = "FieldId";

HtmlDocument htmlDocument = new HtmlDocument();

int fieldId = default( int );

Dictionary<int,string> fieldValuesTable = 
        htmlNode in htmlDocument.DocumentNode.DescendantNodes()
        htmlNode.Name.Equals( ElementToParse, StringComparison.InvariantCultureIgnoreCase )
        htmlNode.Attributes.Contains( IdField )
        id = htmlNode.Attributes[ IdField ].Value
        Int32.TryParse( id.Substring( id.IndexOf( "-" ) + 1 ), out fieldId ) // this is stil not ideal,
        new { Id = fieldId, Text = htmlNode.InnerText }
).ToDictionary( f => f.Id, f => f.Text );

You get the output:

4 : Some text
5 : Some more text

IMHO, it's much cleaner and maintainable.


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.