is this bad practice?

[o35DwA7f]

I am trying to implement sudoku rules in this code for a 2d array. The code works.
Logic tells me i am doing a lot of things wrong, but i feel i am a genius.
I would appreciate some criticism. What am i doing wrong in my efforts or my approach? Is this the best way to apply sudoku rules on a 2d array? How can i break this logic in small little functions who call one another if needed?This question was for how i structure my program from 0 (not this one particularly).You are free to make fun of my code and logic. Thank you very much.

#include <iostream>
#include<iomanip>
using namespace std;
const int boxlength{ 9 };
const int boxheight{ 9 };

void drawmyarray(int array[boxlength][boxheight]);
int enternumber(int array[boxlength][boxheight]);



int main()
{
	int array[boxlength][boxheight]{ 0 };
	int q{0};
	drawmyarray(array);
do 
{//i set a loop for testing it will run once
	q++;
	enternumber(array);
	drawmyarray(array);
} while (q < 1);
	
	return 0;
}

int enternumber(int array[boxlength][boxheight])
{
	int x{ 0 }; int y{ 0 }; int c{0};
	int sumrows{ 0 };
	int sumcolumns{ 0 };
	int sum3x3box{ 0 };
	cout << "enter x,y and then number\n";
	cin >> x >> y >> c;
	array[x][y] = c;
	cout <<"x="<<x<<" y="<<y<<" number="<< array[x][y];
	//here follows 100lines of the same thing more or less
	//in an effort to calcute the sum of all 9 3x3boxes seperately
	if (x < 3 && y < 3)
	{
		for (int i = 0; i < 3; i++)
		{
			for (int j = 0; j < 3; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the first box=" << sum3x3box;
	}
	else if (x < 3 && 2 < y < 6)
	{
		for (int i = 0; i < 3; i++)
		{
			for (int j = 3; j < 6; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the second box=" << sum3x3box;
	}

	else if (x < 3 && 5 < y < 9)
	{
		for (int i = 0; i < 3; i++)
		{
			for (int j = 6; j < 9; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the third box=" << sum3x3box;
	}

	else if (2 < x < 6 && y < 3)
	{
		for (int i = 3; i < 6; i++)
		{
			for (int j = 0; j < 3; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the fourth box=" << sum3x3box;
	}

	else if (2 < x < 6 && 2 < y < 6)
	{
		for (int i = 3; i < 6; i++)
		{
			for (int j = 3; j < 6; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the fifth box=" << sum3x3box;
	}

	else if (2 < x < 6 && 5 < y < 9)
	{
		for (int i = 3; i < 6; i++)
		{
			for (int j = 6; j < 9; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the sixth box=" << sum3x3box;
	}

	else if (5 < x < 9 && y < 3)
	{
		for (int i = 6; i < 9; i++)
		{
			for (int j = 0; j < 3; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the seventh box=" << sum3x3box;
	}

	else if (5 < x < 9 && 2 < y < 6)
	{
		for (int i = 6; i < 9; i++)
		{
			for (int j = 3; j < 6; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the eighth box=" << sum3x3box;
	}

	else if (5 < x < 9 && 5 < y < 9)
	{
		for (int i = 6; i < 9; i++)
		{
			for (int j = 6; j < 9; j++)
			{
				sum3x3box += array[i][j];
			}
		}cout << " the nineth box=" << sum3x3box;
	}
	//i wanted to have different small functions 
        //like the void drawmyarray below
	// but i was trapped in here and there was no way out
	for (int i = x, j = 0; j < boxheight; j++)
	{
		sumrows += array[i][j];

	}cout <<" the sum of rows is="<< sumrows;
	for (int i = 0, j = y; i < boxheight; i++)
	{
		sumcolumns += array[i][j];
	}cout <<" the sum of columns is="<< sumcolumns;
	

			
		
	
	return array[x][y] = c;
}

void drawmyarray(int array[boxlength][boxheight])
{

	for (int i = 0; i < boxlength; i++)
	{
		cout << "\n";
		for (int j = 0; j < boxheight; j++)
			cout << setw(3) << array[i][j];
	}
	cout << "\n";
}

[Handy Andy]

Hello cppmasternoob,

With a quick look what I see:

int enternumber(int array[boxlength][boxheight])  // <--- 1st dimension size is not needed, but OK if you leave. 2nd dimension must have a size.
{
	int row{}; int col{}; int number{};  // <--- The (0) is not necessary, but OK if you leave. A good variable name helps.
	int sumrows{ 0 };
	int sumcolumns{ 0 };
	int sum3x3box{ 0 };

	cout << "enter row col and then number\n";
	cin >> row >> col >> number;

	array[row][col] = number;

	cout <<"row = "<< row << " col = " << col<<" number="<< array[row][col];

	//here follows 100lines of the same thing more or less
	//in an effort to calcute the sum of all 9 3x3boxes seperately
	if (x < 3 && y < 3)  //This line is OK.

        else if (x < 3 && 2 < y < 6)  // <--- This line is not OK. 

In line 9 you enter "row, col and number". The problem is that "row" and "col" could get a value of 9 or greater and this would put it past the end of the array. "row" and "col" can not be greater than 8 and "number" can not be greater than 9.

In line 19 it would be nice if it worked that way, but it does not. I think what you want is:
else if (x < 3 && 2 < y && y < 6). This would be the more proper way to write this, but I can not say it is the correct way to get what you want. It might be.

Just in this little bit of code a good variable name, blank lines along with proper indenting make a big difference in readability. No only for you , but for others. It also makes errors easier to see and find.

Andy

[jonnin]

A few little things:

1) using namespace std is considered bad, there are many posts on that and web articles.
2) suduko is a known sized grid. its a square grid. you don't need a width and height, one variable will do.
3) personal preference, style / not critical: something++ alone is cluttery. eg line 19. move that to the while, eg while(++q <1) or swap it to a for loop using q

4) find a way to functionalize the similar blocks. Never repeat code in this way unless you have a performance critical application, and then only do it if you can prove via profiling that unrolled is significantly faster.

5) comment what you are doing. comment what a function does and what the parameters mean, and what any return value represents. Comment how the logic works. Comment what a block of code is supposed to do.

[o35DwA7f]

thank you Handy Andy. Don't hesitate to point out anything you notice to be out of order in my code.
"else if (x < 3 && 2 < y && y < 6). This would be the more proper way to write this, but I can not say it is the correct way to get what you want. It might be."
i am intrigued..

[Ganado]

Are you trying to map an (x, y) coordinate to one of the 9, 3x3 boxes in sudoku?

You can do like this:

// Example program
#include <iostream>

// x [0, 8]
// y [0, 8]
// output is box number; goes from 0 to 8
// Feel free to add 1 to calculations to make it not 0-based
// Example: cell_to_box(2, 2) == 0
// Example: cell_to_box(4, 3) == 4
int cell_to_box(int x, int y)
{
    return 3 * (y / 3) + x / 3;
}

int main()
{
    for (int y = 0; y < 9; y++)
    {
        for (int x = 0; x < 9; x++)
        {
            std::cout << cell_to_box(x, y) << ' ';   
        }
        std::cout << '\n';
    }
}

0 0 0 1 1 1 2 2 2 
0 0 0 1 1 1 2 2 2 
0 0 0 1 1 1 2 2 2 
3 3 3 4 4 4 5 5 5 
3 3 3 4 4 4 5 5 5 
3 3 3 4 4 4 5 5 5 
6 6 6 7 7 7 8 8 8 
6 6 6 7 7 7 8 8 8 
6 6 6 7 7 7 8 8 8 

This lets you do something like:

// Example program
#include <iostream>

// x [0, 8]
// y [0, 8]
// box number goes from 0 to 8
// Feel free to add 1 to calculations to make it not 0-based
// Example: cell_to_box(2, 2) == 0
// Example: cell_to_box(4, 3) == 4
int cell_to_box(int x, int y)
{
    return 3 * (y / 3) + x / 3;
}

int main()
{
    const char* const ordinals[9] = { "first",  "second", "third",
                                      "fourth", "fifth",  "sixth",
                                      "seventh", "eighth", "ninth" };
    
    int x = 4;
    int y = 3;
    
    int box_index = cell_to_box(x, y);

    std::cout << "the " << ordinals[box_index] << " box\n";
}

the fifth box

But now, you wish to get the lower/upper bounds of each box.
But you already know the x/y coordinates, so for example, (4, 3) is the 4th box,
which starts at x = 3 and continues to x < 6,
and y = 3 and continues to y < 6.

You can use the modulo (%) operator to simplify this.

// Example program
#include <iostream>

// x [0, 8]
// y [0, 8]
// box number goes from 0 to 8
// Feel free to add 1 to calculations to make it not 0-based
// Example: cell_to_box(2, 2) == 0
// Example: cell_to_box(4, 3) == 4
int cell_to_box(int x, int y)
{
    return 3 * (y / 3) + x / 3;
}

int main()
{
    for (int y = 0; y < 9; y++)
    for (int x = 0; x < 9; x++)
    {        
        int lower_x = x - x % 3;
        int upper_x = x - x % 3 + 3;
        
        int lower_y = y - y % 3;
        int upper_y = y - y % 3 + 3;
        
        std::cout << "(" << x << ", " << y << ") --> " << "i:[" << lower_x << ", " << upper_x << "]\n"
                                            << "           j:[" << lower_y << ", " << upper_y << "]\n";
    }

}

(0, 0) --> i:[0, 3]
           j:[0, 3]
(1, 0) --> i:[0, 3]
           j:[0, 3]
(2, 0) --> i:[0, 3]
           j:[0, 3]
(3, 0) --> i:[3, 6]
           j:[0, 3]
(4, 0) --> i:[3, 6]
           j:[0, 3]
(5, 0) --> i:[3, 6]
           j:[0, 3]
(6, 0) --> i:[6, 9]
           j:[0, 3]
(7, 0) --> i:[6, 9]
           j:[0, 3]
...

(4, 8) --> i:[3, 6]
           j:[6, 9]
(5, 8) --> i:[3, 6]
           j:[6, 9]
(6, 8) --> i:[6, 9]
           j:[6, 9]
(7, 8) --> i:[6, 9]
           j:[6, 9]
(8, 8) --> i:[6, 9]
           j:[6, 9]

If you apply this, you can avoid most of your if-statements, and you can turn your for loops into

		int lower_x = x - x % 3;
		int upper_x = x - x % 3 + 3;
        
		int lower_y = y - y % 3;
		int upper_y = y - y % 3 + 3;

		for (int i = lower_y; i < upper_y; i++)
		{
			for (int j = lower_x; j < upper_x; j++)
			{
				sum3x3box += array[i][j];
			}

[dutch]

    else if (x < 3 && 2 < y < 6)

You can't say 2 < y < 6 (or at least it doesn't do what you think).
It's interpretted as (2 < y) < 6, where 2 < y is replaced by 0 or 1 depending on the truth value, then the 0 or 1 is compared to 6.
It should be:

    else if (x < 3 && 2 < y && y < 6)

However, I believe you would like a way to shorten this code.
How about:

#include <iostream>
#include <iomanip>
using namespace std;

const int Height = 9, Width = 9;

void enterValue(int board[][Width])
{
    int row, col, n;
    cout << "enter row, column and value, separated by spaces\n";

    while (true)
    {
        cin >> row >> col >> n;
        if (row < 0 || row >= Height)
            cout << "bad row\n";
        else if (col < 0 || col >= Width)
            cout << "bad column\n";
        else if (board[row][col] != 0)
            cout << "that position is already assigned\n";
        else if (n < 1 || n > 9)
            cout << "value must be from 1 to 9\n";
        else
            break;
    }

    board[row][col] = n;

    int rowStart = (row / 3) * 3;
    int colStart = (col / 3) * 3;
    int sum = 0;
    for (int r = rowStart; r < rowStart + 3; ++r)
        for (int c = colStart; c < colStart + 3; ++c)
            sum += board[r][c];
    cout << "box sum: " << sum << '\n';

    sum = 0;
    for (int r = 0; r < Height; ++r)
        sum += board[r][col];
    cout << "row sum: " << sum << '\n';

    sum = 0;
    for (int c = 0; c < Width; ++c)
        sum += board[row][c];
    cout << "column sum: " << sum << '\n';
}

void drawBoard(int board[][Width])
{
    for (int row = 0; row < Height; ++row)
    {
        for (int col = 0; col < Width; ++col)
            cout << setw(2) << board[row][col] << ' ';
        cout << '\n';
    }
}

int main()
{
    int board[Height][Width] { { 0 } };
    drawBoard(board);

    while (true)
    {
        enterValue(board);
        drawBoard(board);
    }
}

[o35DwA7f]

thank you all very much. Handy Andy, jonnin, Ganado, dutch thank you all for your time and answers. You are always helpful. I got that 2<x<5 is ridiculous, i know about namespaces, the rest i can't understand at the moment i need some time because my big brain is melting.

[jonnin]

that thingy ganado asked about... looks like a kronoker tensor product (spelling?), or part of one. There may be some linear algebra tricks depending on what you are trying to accomplish.

[TheIdeasMan]

I am not sure what the point of summing a partially filled board is? Your enternumber function only puts 1 number on the board, then all the sums are calculated for that 1 number? Even for a partially filled board (say 20 numbers) summing doesn't make sense IMO.

However, the advice you have received so far will still be useful.

What you need is two 9 by 9 data structures. The first has the numbers that are on the board at the beginning.

The second has a list of the 9 candidate integers for each of the 81 cells. Candidates are removed if that number exists in the row, column or 3 by 3 box. This is the starting point for sudoku: implementing the other strategies like only position row, only position box, locked pairs, locked triples etc follows on from here. Once there is only 1 candidate left, that is the number which will be placed in that cell.

My other advice is to consider using c++ containers like std::vector rather than plain arrays. If you want to learn c++, then write in c++.

Good Luck :+)