is this bad practice?

closed account (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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
 
#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 (5051)
Hello cppmasternoob,

With a quick look what I see:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
 
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 (11497)
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.
Last edited on
closed account (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 (6862)
Are you trying to map an (x, y) coordinate to one of the 9, 3x3 boxes in sudoku?

You can do like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
 
// 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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
 
// 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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
 
// 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
1
2
3
4
5
6
7
8
9
10
11
12
 
		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];
			}
Last edited on
dutch (2548)
 
 
    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:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
 
#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);
    }
}

closed account (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 (11497)
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.
Last edited on
TheIdeasMan (6847)
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 :+)
Topic archived. No new replies allowed.