HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Searching all diagonals of a 2D M x M array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arraydiagonalssearchingall

Problem

I've started writing a piece of code to help me search for an object in all the objects found in the diagonals of an M x M 2D array.

Though the code works, I'd like to know if there is a way I can improve on it or there exists a different technique I could use to achieve the same result with fewer lines of code. I just don't want to re-invent the wheel.

// board must be a square matrix    
bool allDiagonals(string[] board, string strToFind)
{
    string s = "";
    for (int col = board[0].Length - 1; col >= 0; col--)
    {

        int row = 0;
        int j = col;
        s = "";
        while (j  0; row--)
    {
        int colmin = 0;
        s = "";
        for (int i = row; i = 0)
        {
            s += board[row++][j--];
        }
        if (s.Contains(strToFind))
            return true;
    }
    for (int row = board.Length - 1; row > 0; row--)
    {
        s = "";
        int colmax = board[0].Length - 1;
        //scan from bottom right to main diagonal
        for (int i = row; i < board.Length; i++)
        {
            s += board[i][colmax--];
        }
        if (s.Contains(strToFind))
            return true;
    }
    return false;
}

Solution

This reminds me of what I wrote a while ago for my Tic Tac Toe Ultimate game.

In case I get anything wrong, here is a relevant part of my original code:

// Scan diagonals for a winner: Bottom-right
for (int yy = 0; yy  loopAdd(HasSub board,
        int xx, int yy, int dx, int dy) {
    List winnables = new ArrayList<>();

    Winnable tile;
    do {
        tile = board.getSub(xx, yy);
        xx += dx;
        yy += dy;
        if (tile != null)
            winnables.add(tile);
    }
    while (tile != null);

    return winnables;
}


When making this in C#, you can return IEnumerable. The idea of the method is to start at a specific point and to iterate until you're outside the valid area.

I think it's a bad idea that you're currently using String concatenation first, and then checking if the string contains the char. Instead you can indeed as Carsten points out in the comments use a state machine for that. It only needs to be about an int really that keeps track of how many consecutive matches you've had so far, and when it reaches a non-match it starts over at zero. When it reaches the string length, it returns true.

Anyways, back to the main part, here's what I would try to do:

private IEnumerable loopOver(string[] board,
        Point position, Point delta) {

    char value;
    while (true) {
        if (!charIsInsideBoard(board, position))
            break;

        value = getCharInBoard(board, position);
        position.x += delta.x;
        position.y += delta.y;
        yield return value;
    }
}


Whenever I'm using C#, I love using the wonderful yield keyword.

private boolean contains(IEnumerable loop, string searchFor) {
    int matches = 0;
    foreach (char ch in loop) {
        if (ch == searchFor[matches]) {
             matches++;
        }
        else {
             matches = 0;
             if (ch == searchFor[matches]) {
                 matches++;
             }
        }
        if (matches == searchFor.Length) {
             return true;
        }
    }
    return false;
}


Then in your allDiagonals method (which really could be renamed to searchAllDiagonals) all you need to do is to call the contains and loopOver methods a couple of times.

boolean searchAllDiagonals(string[] board, string searchFor)
{
    boolean result = false;
    for (int i = 0; i < board[0].Length; i++) {
        result = result || contains(loopOver(board, new Point(i, 0), new Point(1, 1), searchFor);
    }
    for (int i = 0; i < board.Length; i++) {
        result = result || contains(loopOver(board, new Point(0, i), new Point(1, 0), searchFor);
    }
    ...
}


Please note that I have not tested this code, and as I haven't done any C# development since last time I might have mixed something up. I also might have mixed up X and Y, but that happens for me in Java too every now and then.

Other Suggestions:

-
Don't use i and j when you're using nested loops. row and col are so much better, or x and y. I myself use xx and yy sometimes for loops when I already have another x / y variable.

-
As your variable is named board, what do I know, perhaps your whole point is to check for a winner in a Tic Tac Toe game. If a game is your context, then I'd recommend not using string[]. Use a BoardField[][] variable instead, or preferably wrap such a 2D array in a Board class (which can contain the charIsInsideBoard and getCharInBoard methods).

Code Snippets

// Scan diagonals for a winner: Bottom-right
for (int yy = 0; yy < board.getSizeY(); yy++) {
    newWin(conds, consecutive, loopAdd(board, 0, yy, 1, 1));
}

private static List<Winnable> loopAdd(HasSub<? extends Winnable> board,
        int xx, int yy, int dx, int dy) {
    List<Winnable> winnables = new ArrayList<>();

    Winnable tile;
    do {
        tile = board.getSub(xx, yy);
        xx += dx;
        yy += dy;
        if (tile != null)
            winnables.add(tile);
    }
    while (tile != null);

    return winnables;
}
private IEnumerable<char> loopOver(string[] board,
        Point position, Point delta) {

    char value;
    while (true) {
        if (!charIsInsideBoard(board, position))
            break;

        value = getCharInBoard(board, position);
        position.x += delta.x;
        position.y += delta.y;
        yield return value;
    }
}
private boolean contains(IEnumerable<char> loop, string searchFor) {
    int matches = 0;
    foreach (char ch in loop) {
        if (ch == searchFor[matches]) {
             matches++;
        }
        else {
             matches = 0;
             if (ch == searchFor[matches]) {
                 matches++;
             }
        }
        if (matches == searchFor.Length) {
             return true;
        }
    }
    return false;
}
boolean searchAllDiagonals(string[] board, string searchFor)
{
    boolean result = false;
    for (int i = 0; i < board[0].Length; i++) {
        result = result || contains(loopOver(board, new Point(i, 0), new Point(1, 1), searchFor);
    }
    for (int i = 0; i < board.Length; i++) {
        result = result || contains(loopOver(board, new Point(0, i), new Point(1, 0), searchFor);
    }
    ...
}

Context

StackExchange Code Review Q#56092, answer score: 5

Revisions (0)

No revisions yet.