You have some "Magic Numbers". If you wanted to work with a different grid size you'd have to replace
9
at many different places. Find the relationships between your hard-coded numbers and declare them asconst
. For example,const int Size = 9;
andconst int Box = 3;
I like that you are using
yield
Your code is quite hard to understand, even with your comments. This can have something to do with your variable names probably.
ToCheck
for example, if that is checking rows, columns, values, what kind of values, etc. is not clear at all from the name. From what I understand, you are sometimes considering the array indexes as index of a 3x3 grid, and sometimes as a row/column. That's confusing for me.Instead of using integer arrays, you could use
Set<int>
.int[][] ToCheck
andint[] ToCheckPointers
should be local variables inside theOutputArrayPoints
method, they do not seem to be used outside it.As you are not incrementing the
c
variable on every iteration, I would use awhile (c >= 0)
rather thanfor
.Slightly related is my Code Review question for a Sudoku Solver my Code Review question for a Sudoku Solver, it might give you an idea of how things can be organized into a more object-oriented approach
All things considered, I must say that your code seems to be quite fast. I am quite sure it can be made faster but it's hard to tell how exactly because of the lack of clarity of the code.
You have some "Magic Numbers". If you wanted to work with a different grid size you'd have to replace
9
at many different places. Find the relationships between your hard-coded numbers and declare them asconst
. For example,const int Size = 9;
andconst int Box = 3;
I like that you are using
yield
Your code is quite hard to understand, even with your comments. This can have something to do with your variable names probably.
ToCheck
for example, if that is checking rows, columns, values, what kind of values, etc. is not clear at all from the name. From what I understand, you are sometimes considering the array indexes as index of a 3x3 grid, and sometimes as a row/column. That's confusing for me.Instead of using integer arrays, you could use
Set<int>
.int[][] ToCheck
andint[] ToCheckPointers
should be local variables inside theOutputArrayPoints
method, they do not seem to be used outside it.As you are not incrementing the
c
variable on every iteration, I would use awhile (c >= 0)
rather thanfor
.Slightly related is my Code Review question for a Sudoku Solver, it might give you an idea of how things can be organized into a more object-oriented approach
All things considered, I must say that your code seems to be quite fast. I am quite sure it can be made faster but it's hard to tell how exactly because of the lack of clarity of the code.
You have some "Magic Numbers". If you wanted to work with a different grid size you'd have to replace
9
at many different places. Find the relationships between your hard-coded numbers and declare them asconst
. For example,const int Size = 9;
andconst int Box = 3;
I like that you are using
yield
Your code is quite hard to understand, even with your comments. This can have something to do with your variable names probably.
ToCheck
for example, if that is checking rows, columns, values, what kind of values, etc. is not clear at all from the name. From what I understand, you are sometimes considering the array indexes as index of a 3x3 grid, and sometimes as a row/column. That's confusing for me.Instead of using integer arrays, you could use
Set<int>
.int[][] ToCheck
andint[] ToCheckPointers
should be local variables inside theOutputArrayPoints
method, they do not seem to be used outside it.As you are not incrementing the
c
variable on every iteration, I would use awhile (c >= 0)
rather thanfor
.Slightly related is my Code Review question for a Sudoku Solver, it might give you an idea of how things can be organized into a more object-oriented approach
All things considered, I must say that your code seems to be quite fast. I am quite sure it can be made faster but it's hard to tell how exactly because of the lack of clarity of the code.
You have some "Magic Numbers". If you wanted to work with a different grid size you'd have to replace
9
at many different places. Find the relationships between your hard-coded numbers and declare them asconst
. For example,const int Size = 9;
andconst int Box = 3;
I like that you are using
yield
Your code is quite hard to understand, even with your comments. This can have something to do with your variable names probably.
ToCheck
for example, if that is checking rows, columns, values, what kind of values, etc. is not clear at all from the name. From what I understand, you are sometimes considering the array indexes as index of a 3x3 grid, and sometimes as a row/column. That's confusing for me.Instead of using integer arrays, you could use
Set<int>
.int[][] ToCheck
andint[] ToCheckPointers
should be local variables inside theOutputArrayPoints
method, they do not seem to be used outside it.As you are not incrementing the
c
variable on every iteration, I would use awhile (c >= 0)
rather thanfor
.Slightly related is my Code Review question for a Sudoku Solver, it might give you an idea of how things can be organized into a more object-oriented approach
All things considered, I must say that your code seems to be quite fast. I am quite sure it can be made faster but it's hard to tell how exactly because of the lack of clarity of the code.