Refactor Ifs nesting statements in C#

I would like refactoring my code for do it more elegant andclear, if it is possible to reduce nesting “if” statements.

My code "parsers" any string (results) of MsBuil process for check if process is correct build.

public static bool CheckMsBuildResult(string resultadoEjecucionScriptMsBuild)
        {
        // Build started 01/09/2010 8:54:07.
        string s1 = @"Build Started \d{1,2}/\d\d/\d{4} \d{1,2}:\d\d:\d\d";

        //Build started 9/1/2010 10:53:35 AM.
        //Build started 9/1/2010 8:42:16 AM.
        string s1Mod = @"Build Started \d{1,2}/\d{1,2}/\d{4} \d{1,2}:\d\d:\d\d";

        s1 = s1Mod;

        string s11 = @"n Generar iniciada a las \d{1,2}/\d\d/\d{4} \d{1,2}:\d\d:\d\d";

        // Compilaci�n iniciada a las 28/02/2011 14:56:55.
        string s12 = @"Compilaci.n iniciada a las \d{1,2}/\d\d/\d{4} \d{1,2}:\d\d:\d\d";


        string s2 = "Build succeeded.";
        string s21 = @"Generaci.n satisfactoria\.";
        string s3 = @"0 Error\(s\)";
        string s31 = "0 Errores";

        Regex rg = new Regex(s1, RegexOptions.Multiline | RegexOptions.IgnoreCase);
        Match mt = rg.Match(resultadoEjecucionScriptMsBuild);
        if (!mt.Success)
        {
            rg = new Regex(s11, RegexOptions.Multiline | RegexOptions.IgnoreCase);
            mt = rg.Match(resultadoEjecucionScriptMsBuild);
            if (!mt.Success)
            {
                rg = new Regex(s12, RegexOptions.Multiline | RegexOptions.IgnoreCase);
                mt = rg.Match(resultadoEjecucionScriptMsBuild);

                if (!mt.Success) return false;
            }
        }

        int i = mt.Index + mt.Length;

        rg = new Regex(s2, RegexOptions.Multiline | Reg开发者_C百科exOptions.IgnoreCase);
        mt = rg.Match(resultadoEjecucionScriptMsBuild, i);
        if (!mt.Success)
        {
            rg = new Regex(s21, RegexOptions.Multiline | RegexOptions.IgnoreCase);
            mt = rg.Match(resultadoEjecucionScriptMsBuild);
            if (!mt.Success)
            {
                return false;
            }
        }

        i = mt.Index + mt.Length;

        rg = new Regex(s3, RegexOptions.Multiline | RegexOptions.IgnoreCase);
        mt = rg.Match(resultadoEjecucionScriptMsBuild, i);
        if (!mt.Success)
        {
            rg = new Regex(s31, RegexOptions.Multiline | RegexOptions.IgnoreCase);
            mt = rg.Match(resultadoEjecucionScriptMsBuild);
            if (!mt.Success)
            {
                return false;
            }
        }

        return true;
        }

---

It looks like you could just reduce this down to one or two lines of code by using a single more powerful regex instead of the s1, s1mod, s12, etc. However, since you've redacted your regexes I can't advise exactly what that would look like.

I would also criticise this code for

• Undescriptive variable names.
• Unnecessary re-use of variables
• Duplication
• Poor method name (check what, exactly?)
• Potentially poor performance if this method is called often, due to the lack of re-use of compiled regexes.

---

First refactor variable names. Then try to flatten code using booleans, like in this sample:

    Regex rg = new Regex(s1, RegexOptions.Multiline | RegexOptions.IgnoreCase);
    bool match_s1 = rg.Match(resultadoEjecucionScriptMsBuild).Success;        

    rg = new Regex(s11, RegexOptions.Multiline | RegexOptions.IgnoreCase);
    bool match_s11 = rg.Match(resultadoEjecucionScriptMsBuild).Success;         

    rg = new Regex(s12, RegexOptions.Multiline | RegexOptions.IgnoreCase);        
    bool match_s12 = rg.Match(resultadoEjecucionScriptMsBuild).Success;                 

    if (!match_s1 && !match_s11) 
        return match_s12; 

---

Here are two ideas:

• It seems like you're trying to encode decision tree using nested if. A better alternative could be to define a data structure that encodes the tree and then recursively traverse the tree to test the match.

• I don't think there is any really simple way to refactor the code to avoid nesting. If you're brave enough, you could use LINQ syntax. (There is a free chapter from my functional programming book that explains how monads work and how to write thing like this using LINQ).

Using the LINQ syntax, you could rewrite this:

Regex rg = new Regex(s1, RegexOptions.Multiline | RegexOptions.IgnoreCase);
Match mt = rg.Match(resultadoEjecucionScriptMsBuild);
if (!mt.Success)
{
    rg = new Regex(s11, RegexOptions.Multiline | RegexOptions.IgnoreCase);
    mt = rg.Match(resultadoEjecucionScriptMsBuild);
    if (!mt.Success)
    {
        rg = new Regex(s12, RegexOptions.Multiline | RegexOptions.IgnoreCase);
        mt = rg.Match(resultadoEjecucionScriptMsBuild);
        if (!mt.Success) return false;
    }
}

... into something like this (Note that you'd need to define quite a few helpers and I'm not showing the full code - so you cannot compare the length - but you can see that it avoids nesting):

var matches = 
    from rg1 in MatchRegex(s1, RegexOptions.Multiline | RegexOptions.IgnoreCase,
                          resultadoEjecucionScriptMsBuild)
    from rg2 in MatchRegex(s11, RegexOptions.Multiline | RegexOptions.IgnoreCase,
                          resultadoEjecucionScriptMsBuild)
    from rg3 in MatchRegex(s11, RegexOptions.Multiline | RegexOptions.IgnoreCase,
                          resultadoEjecucionScriptMsBuild)
    select true;

if (matches) return false;

This could be useful especially if you needed to access some of the earlier Match objects to determine the final result (e.g. use rg1 at the end). Otherwise, you could probably just write a method that runs a given regex and combine them in a single big if statement using &&.

---

I think you can reduce a lot of duplicate code here.

EDIT (Removed some more duplicate code and posted a linq method):

public static Match MatchOptions(string resultadoEjecucionScriptMsBuild, int index, params string[] strings)
{
    return strings.Select(s => new Regex(s, RegexOptions.Multiline | RegexOptions.IgnoreCase))
        .Select(rg => rg.Match(resultadoEjecucionScriptMsBuild, index)).FirstOrDefault(mt => mt != null);
}

or if you don't like linq:

public static Match MatchOptions(string resultadoEjecucionScriptMsBuild, int index, params string[] strings)
{
    foreach (string s in strings)
    {
        Regex rg = new Regex(s, RegexOptions.Multiline | RegexOptions.IgnoreCase);
        Match mt = rg.Match(resultadoEjecucionScriptMsBuild, index);

        if (mt != null)
            return mt;
    }

    return null;
}

public static bool CheckMsBuildResult(string resultadoEjecucionScriptMsBuild)
{
    string s1 = @" ..... ";
    string s1Mod = @" ..... ";
    string s11 = @" ..... ";

    // Compilaci�n iniciada a las 28/02/2011 14:56:55.
    string s12 = @" ..... ";

    string s2 = @" ..... ";
    string s21 = @" ..... ";
    string s3 = @" ..... ";
    string s31 = @" ..... ";

    var stringsArray = new[]
                        {
                            new[] {s1, s11, s12},
                            new[] {s2, s21},
                            new[] {s3, s31}
                        };

    Match mt = null;
    int i = 0;
    foreach (string[] strings in stringsArray)
    {
        mt = MatchOptions(resultadoEjecucionScriptMsBuild, i, strings);
        if (mt == null)
            return false;

        i = mt.Index + mt.Length;
    }

    return mt != null;
}

---

It seems that you want to test, if the input consists of one of s1|s11|s12 followed by one of s2|s21 and finally one of s3|s31.

You can always make a more complex regular expression which reflects this using:

var complex = "((" + s1 + ")|(" + s11 + ")|(" + s12 + "))|((" + s2 + ")|(" + s21 + ...

and using this in a single match.

If you just want to get rid of the nested ifs (i.e. if you want a refactoring which maintains the current control flow) I'd group the regular expressions into lists like

var reList = new List<List<string>>() {
  new List<string>(){s1, s11, s12},
  new List<string>(){s2, s21},
  new List<string>(){s3, s31}
};

(using C# 3.0 collection initializers)

Then you can iterate over all expressions in two nested loops like

var i = 0;
foreach (var outer in reList) {
  Match mt;
  foreach (var inner in outer) {
    rg = new Regex(inner, RegexOptions.Multiline | RegexOptions.IgnoreCase);
    mt = rg.Match(resultadoEjecucionScriptMsBuild, i);
    if (mt.Success)
      break;
  }
  if (!mt.Success)
    return false;
  i = mt.Index + mt.Length;
}
return true;

This function returns true, if the input string is a concatenation of one of s1,s11,s12, one of s2, s21 and finally one of s3, s31.