Regex - Input Checker Class - Lambda with two lists

Question

I have an input checker class with some RegExes, and I have converted some of the loops to lambdas. Am I doing it right? They seem to work fine, but I'm new to Lambdas. Does this offer a performance benefit over the commented implementation? Should I try implementing the first one as a lambda, and if so, how? Thanks in advance!

public static class DataChecker
{
    public static bool IsGameInputDataValid(string[] dataFile)
    {
        if (dataFile.Length < 5)
        {
            return false;
        }

        var regexList = new List<Regex>();
        regexList.Add(new Regex("^\\d\\s\\d$"));
        regexList.Add(new Regex("^(\\d,\\d )*(\\d,\\d)$"));
        regexList.Add(new Regex("^\\d\\s\\d$"));
        regexList.Add(new Regex("^\\d \\d [NESW]{1}$"));
        regexList.Add(new Regex("^[RLM]{1}( [RLM]{1})*$"));

        //cant figure this one out as lambda... but should it even be converted for performance reasons?
        for (int i = 0; i < 4; i++)
        {
            if (!regexList[i].IsMatch(dataFile[i]))
            {
                return false;
            }
        }

        var listOfMoves = dataFile.Select(x => x).Skip(4).ToList();
        //instead of:
        // for (int i = 4; i<countOfLinesInInputFile; i++)
        // {
        //     listOfMoves.Add(lines[i]);
        // }

        var areaAnyMoveSequencesInvalid =
            listOfMoves.Select(x => regexList[4].IsMatch(x)).ToList()
            .Any(y => y == false);

        if (areaAnyMoveSequencesInvalid)
        {
            return false;
        }

        // instead of:
        // foreach (var line in listOfMoves)
        // {
        //     if (!regexList[4].IsMatch(line))
        //     {
        //         return false;
        //     }
        // }

        return true;
    }
}

edit: Originally I wanted the question to only focus on the lambdas, but I got a lot of responses for the RegExes as well. Therefore I'll add the missing parts to make sense of that. It's a turtle-moving game in 2-dimensional x,y coordinates map.

Sample data file:

5 4
1,1 1,3 3,3
4 2
0 1 N
R M L M M
R M M M
M R M M M M
M R M M
M M M

Rules: The first line should define the board size • The second line should contain a list of mines (i.e. list of co-ordinates separated by a space) • The third line of the file should contain the exit point. • The fourth line of the file should contain the starting position of the turtle. • The fifth line to the end of the file should contain a series of moves.

Where: R: Rotate right L: Rotate left M: Move N,E,S,W: cardinal directions

I've updated the original with d+ instead of d, to allow for multiple digit coordinates:

        regexList.Add(new Regex("^\\d+\\s\\d+$"));
        regexList.Add(new Regex("^(\\d+,\\d+ )*(\\d+,\\d+)$|^$"));
        regexList.Add(new Regex("^\\d+\\s\\d+$"));
        regexList.Add(new Regex("^\\d+ \\d+ [NESW]{1}$"));
        regexList.Add(new Regex("^[RLM]{1}( [RLM]{1})*$"));

Answer 1

I would tackle this problem by not adding the last regex to the list but to have this as a separate regex. Using a little "trick" by using Enumerable.Range() you can replace the for with a linq expression like so

if (Enumerable.Range(0, 4).Any(i => !regexList[i].IsMatch(dataFile[i]))) { return false; }

After the loop you have this line

var listOfMoves = dataFile.Select(x => x).Skip(4).ToList();  

in which you neither need the call to Select() nor to ToList().

In the next line

var areaAnyMoveSequencesInvalid =
            listOfMoves.Select(x => regexList[4].IsMatch(x)).ToList()
            .Any(y => y == false);  

A simple call to Any() would be sufficient but you could just return the value of a call to All() instead like shown below.

If the method in question is called quite often you should consider to use compiled regex. They take a little bit more time to create them but executing them will become faster. If you take this way you should initialize the regexes in the static constructor.

Summing this up will lead to

public static class DataChecker
{
    static DataChecker()
    {
        regexList = new List<Regex>();
        regexList.Add(new Regex("^\\d\\s\\d$", RegexOptions.Compiled));
        regexList.Add(new Regex("^(\\d,\\d )*(\\d,\\d)$", RegexOptions.Compiled));
        regexList.Add(new Regex("^\\d\\s\\d$", RegexOptions.Compiled));
        regexList.Add(new Regex("^\\d \\d [NESW]{1}$", RegexOptions.Compiled));

        lastRegex = new Regex("^[RLM]{1}( [RLM]{1})*$", RegexOptions.Compiled);
    }
    private static readonly List<Regex> regexList;
    private static readonly Regex lastRegex;
    public static bool IsGameInputDataValid(string[] dataFile)
    {
        if (dataFile.Length < 5) { return false; }

        if (Enumerable.Range(0, 4).Any(i => !regexList[i].IsMatch(dataFile[i]))) { return false; }

        return dataFile.Skip(4).All(x => lastRegex.IsMatch(x));
    }
}

Without knowing what the input looks like I can't come up with a better name for the last regex. In addition this isn't tested but should work like a charme.

Answer 2

When working with the Regex pattern, you can benefit from the "verbatim string literal" operator @:

so that

"^\\d\\s\\d$" becomes @"^\d\s\d$"

This is more readable and if you're using Visual Studio, you will get help from intellisense (a better coloring), when designing the pattern directly in place of new Regex(pattern)

---

Besides that, I'd go with the suggestions by Heslacher, but take it a step further:

  public static class DataCheckerReview
  {
    static readonly List<Regex> headerPatterns;
    static readonly Regex movePattern;

    static DataCheckerReview()
    {
      headerPatterns = new List<Regex>
      {
        new Regex(@"^\d\s\d$", RegexOptions.Compiled),
        new Regex(@"^(\d,\d )*(\d,\d)$", RegexOptions.Compiled),
        new Regex(@"^\d\s\d$", RegexOptions.Compiled),
        new Regex(@"^\d \d [NESW]$", RegexOptions.Compiled)
      };

      movePattern = new Regex(@"^[RLM]( [RLM])*$", RegexOptions.Compiled);
    }

    static bool LinesEnough(string[] dataLines) => dataLines != null && dataLines.Length > 4;
    static bool CheckHeaders(string[] dataLines) => headerPatterns.Zip(dataLines).All(pair => pair.First.IsMatch(pair.Second));
    static bool CheckMoves(string[] dataLines) => dataLines.Skip(4).All(dl => movePattern.IsMatch(dl));

    public static bool IsGameInputDataValid(string[] dataLines)
    {
      return LinesEnough(dataLines) && CheckHeaders(dataLines) && CheckMoves(dataLines);
    }
  }

When initializing a list "statically" as you do, you can do it as shown above instead of calling Add() for each entry (You can use this approach for all objects that provides an Add() method and implements IEnumerable). If you don't add or remove entries in the list, a plain array may be a better choice than a list?

In CheckHeaders() the Zip() extension is used to pair each regex in the header patterns with the first four lines in the input lines. Zip() only pairs items in the sets for the count of items in the shortest set, so no need for Take() etc.

By abstracting the validation of the input away in dedicated methods, you get a clean design, that is easy to maintain and the naming makes it easy to understand what each step does.

The context is somewhat unclear, but just returning bool from a method that checks several properties is not very informative. I think, I'd throw a dedicated exception if a step fails or at least have some kind of logging. As a client of the method, it would be nice to know what went wrong.

Answer 3

Disclaimer: Not a Code Reviewer

• Just looking into the regular expressions here!

• Not sure exactly what we are doing.

• My guess is that:

We would probably want:

    regexList.Add(new Regex("^(?:\\d,\\d )*(?:\\d,\\d)$"));

instead of

    regexList.Add(new Regex("^(\\d,\\d )*(\\d,\\d)$"));

and we don't want the {1}:

    regexList.Add(new Regex("^\\d \\d [NESW]$"));
    regexList.Add(new Regex("^[RLM]( [RLM])*$"));

and maybe we would want non-capturing groups here too, instead of capturing groups:

    regexList.Add(new Regex("^\\d \\d [NESW]$"));
    regexList.Add(new Regex("^[RLM](?: [RLM])*$"));

and that'd a bit simplify the expressions.

Happy Coding! ( ˆ_ˆ )