5
\$\begingroup\$

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})*$"));
\$\endgroup\$
9
  • 1
    \$\begingroup\$ Welcome to the Code Review Site. The question title should tell us what the code does rather than what your concerns about the code are. It would also be helpful if there was a short paragraph explaining what the code does, this helps us provide a better review and better suggestions. Please read How do I ask a good question, it can give you some hints on the title and other good tips for getting up votes on your question. You might also want to remove any commented out code because that type of code makes us question if the code is ready \$\endgroup\$
    – pacmaninbw
    Oct 30, 2020 at 23:36
  • \$\begingroup\$ perhaps given us a sample of dataFile and explaination on what the expressions do and what data do you consider Valid. This would give us some idea on your question and we can help you with that. \$\endgroup\$
    – iSR5
    Oct 31, 2020 at 5:28
  • \$\begingroup\$ Please don't ignore comments that advise you to improve your post in any way, @pacmaninbw suggested something because those are the traits of a good question. I have edited the title for you now \$\endgroup\$
    – user228914
    Nov 2, 2020 at 6:34
  • \$\begingroup\$ Hi! Sorry for getting back to the thread a little late and thanks so much for all the replies! The original idea was to not put the emphasis on reviewing the regexes for, but the lambdas -- I only included the regexes so you can see what I'm working with later on. However, I'll edit the post now because of the replies :) \$\endgroup\$ Nov 4, 2020 at 16:25
  • 1
    \$\begingroup\$ Please do not update your question after you have received answers: codereview.stackexchange.com/help/someone-answers I've rolled back your edit. If you want to post a follow-up question, you can do so. \$\endgroup\$
    – BCdotWEB
    Nov 4, 2020 at 16:28

3 Answers 3

5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ The first 4 Regex's are the mystery to me. The last one is a way to validate a "move". \$\endgroup\$
    – Rick Davin
    Nov 2, 2020 at 13:06
  • \$\begingroup\$ This is exactly what I was looking for! Thanks so much! I've added a sample file and the rules for the regexes for more clarification. \$\endgroup\$ Nov 4, 2020 at 16:26
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks very much for the suggestions, sounds like some valid points there! \$\endgroup\$ Nov 4, 2020 at 16:30
2
\$\begingroup\$

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! ( ˆ_ˆ )

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks for reviewing the RegExes! :) I originally did not want to focus on the regexes much, but I have added some clarifications to the OP for them now. I agree about the non-capturing groups (According to StackOverflow, it should make a difference performance-wise (albeit small in this case)). The {1} is needed due to the rules I've just posted I believe. \$\endgroup\$ Nov 4, 2020 at 16:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.