Print a matrix of alternating X's and O's given column and row constraints?

Question

I'm trying to write an algorithm that prints a matrix of X's and O's, given the following params:

int numRows
int numCols
int charsPerCol
int charsPerRow

e.g. calling

printXOMatrix(int charsPerCol, int charsPerRow, int numCols, int numRows);

with the parameters

printXOMatrix(3,2,15,8);

will result in the following being printed to stdout:

XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO

Here's my code so far, it seems to print correctly if the number of columns / the number of chars per column is different, but fails for example in the case of:

printXOMatrix(2,2,8,8);

the following is printed to stdout:

XXOOXXOO
OOXXOOXX
OOXXOOXX
XXOOXXOO
XXOOXXOO
OOXXOOXX
OOXXOOXX
XXOOXXOO

How do I handle this edge case/clean up my code? Here's what I have so far:

#include <stdio.h>

void getXAndOGrid(int charsPerCol, int charsPerRow, int numCols, int numRows) {
    char c = 'X';
        for (int i=1; i<=(numCols*numRows); i++) {
            // if current index is divisible by the columns (new row)
            if (i % numCols == 0) {
                // print character, then newline
                printf("%c\n", c);
                // if current index is divisible by number of columns times num of chars in column
                if (i % (numCols * charsPerRow) == 0) {
                    if (c == 'O') {
                        c = 'X';
                    } else {
                        c = 'O';
                    }
                }
            // else if current index is divisible by num in row before it alternates
            // and is not divisible by number of columns
            } else if (i % charsPerCol == 0) {
                printf("%c", c);
                if (c == 'O') {
                    c = 'X';
                } else {
                    c = 'O';
                }
            } else {
                printf("%c", c);
            }
        }
}

int main() {
    getXAndOGrid(3,2,15,8);
    return 0;
}

Answer 1

Well, after figuring out the code had an inconsistancy as selbie said, it seems the problem is when you reach the end of the line you don't reset c back to what it was at the start of the line!

But the question should be: why are you writing so much code? The following does exactly the same (well, apart from handling the edge case correctly!):-

void PrintMatrix (int charsPerCol, int charsPerRow, int numCols, int numRows)
{
  for (int y = 0 ; y < numRows ; ++y)
  {
    for (int x = 0 ; x < numCols ; ++x)
    {
      printf ((((x / charsPerCol) & 1) ^ ((y / charsPerRow) & 1)) != 0 ? "o" : "x");
    }

    printf ("\n");
  }
}

Answer 2

The problem is that you missed one case. You handled the case that the table switch up, with the condition: if (i % (numCols * charsPerRow) == 0), but you haven't handled the case when it doesn't . So I add another condition:

if (i % (numCols * charsPerRow) == 0) 
{

    if ((numCols / charsPerCol)%2 == 1)
    {
        if (c == 'O')
        {
            c = 'X';
        }
        else
        {
            c = 'O';
        }
    }
}
else
{
    if ((numCols / charsPerCol)%2 == 0)
    {
        if (c == 'O')
        {
            c = 'X';
        }
        else
        {
            c = 'O';
        }
    }
}

So, if the (numCols / charsPerCol) is odd, for instance XXOOXXOOXX, no switch from X <-> O needed, otherwise yes (for instance XXOOXXOO, now c need to be switched from O -> X).

The opposite holds for when the initial condition i % (numCols * charsPerRow) == 0 is reached.

Full code:

#include <iostream>
using namespace std;

void getXAndOGrid(int charsPerCol, int charsPerRow, int numCols, int numRows) {
    char c = 'X';
        for (int i=1; i<=(numCols*numRows); i++) {
            // if current index is divisible by the columns (new row)
            if (i % numCols == 0) {
                // print character, then newline
                printf("%c\n", c);

                // if current index is divisible by number of columns times num of chars in column
                if (i % (numCols * charsPerRow) == 0) {

                    if ((numCols / charsPerCol)%2 == 1)
                    {
                        if (c == 'O') {
                        c = 'X';
                        } else {
                            c = 'O';
                        }
                    }
                }
                else
                {
                    //cerr << c << endl;
                    if ((numCols / charsPerCol)%2 == 0)
                    {
                        if (c == 'O') {
                        c = 'X';
                        } else {
                            c = 'O';
                        }
                    }
                }
            // else if current index is divisible by num in row before it alternates
            // and is not divisible by number of columns
            }
            else if (i % charsPerCol == 0) {
                printf("%c", c);
                if (c == 'O') {
                    c = 'X';
                } else {
                    c = 'O';
                }
            }
            else {
                printf("%c", c);
            }
        }
}

int main() {
    getXAndOGrid(2,2,8,8);
    return 0;
}

Output (your case):

XXOOXXOO
XXOOXXOO
OOXXOOXX
OOXXOOXX
XXOOXXOO
XXOOXXOO
OOXXOOXX
OOXXOOXX

Output (with getXAndOGrid(3,3,15,15);):

XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO
OOOXXXOOOXXXOOO
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX
XXXOOOXXXOOOXXX

P.S : IMO, your code has quite a bit of duplicated code, wouldn't it be cleaner if a function is implemented?

void switchC(char &c)
{
    if (c == 'O') { c = 'X';} else {c = 'O';}
}

void getXAndOGrid(int charsPerCol, int charsPerRow, int numCols, int numRows) {
    char c = 'X';
    for (int i=1; i<=(numCols*numRows); i++) {
        if (i % numCols == 0) {
            printf("%c\n", c);
            if (i % (numCols * charsPerRow) == 0) {
                if ((numCols / charsPerCol)%2 == 1) {switchC(c);}
            }
            else {
                if ((numCols / charsPerCol)%2 == 0) {switchC(c);}
            }
        }
        else if (i % charsPerCol == 0) { printf("%c", c); switchC(c);}
        else { printf("%c", c);}
    }
}

P.P.S : @Skizz and @selbie has compacted it even more.

Answer 3

There's a lot of complexity in your code because you are printing character by character, using for-loop index values initialized to 1 instead of 0, and trying to print the entire thing in a single loop.

Here's a cleaner approach. There are two unique lines to print. The one starting with X and the one starting with O. Have one for-loop to build each line type. Then another loop to print each line.

What do you think of this? The code below uses C++, but if you need pure C, replace the new/delete calls with malloc and free as seen in the comments:

void printXOMatrix(int numRows, int numCols, int charsPerCol, int charsPerRow)
{
    char* primary = new char[numCols + 1];    // malloc(sizeof(char)*(numcols+1))
    char* opposite = new char[numCols + 1];   // malloc(sizeof(char)*(numcols+1))

    for (int col = 0; col < numCols; col++)
    {
        char ch = (col % (charsPerCol * 2) < charsPerCol) ? 'X' : 'O';
        primary[col] = ch;
        opposite[col] = (ch == 'X') ? 'O' : 'X';
    }
    primary[numCols] = '\0';
    opposite[numCols] = '\0';

    for (int row = 0; row < numRows; row++)
    {
        char* line = (row % (charsPerRow * 2) < charsPerRow) ? primary : opposite;
        std::cout << line << std::endl; // printf("%s\n", line);
    }
    delete[] primary;    // free(primary)
    delete[] opposite;   // free(opposite)
}