Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  • 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 as const. For example, const int Size = 9; and const 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 and int[] ToCheckPointers should be local variables inside the OutputArrayPoints method, they do not seem to be used outside it.

  • As you are not incrementing the c variable on every iteration, I would use a while (c >= 0) rather than for.

  • 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 as const. For example, const int Size = 9; and const 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 and int[] ToCheckPointers should be local variables inside the OutputArrayPoints method, they do not seem to be used outside it.

  • As you are not incrementing the c variable on every iteration, I would use a while (c >= 0) rather than for.

  • 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 as const. For example, const int Size = 9; and const 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 and int[] ToCheckPointers should be local variables inside the OutputArrayPoints method, they do not seem to be used outside it.

  • As you are not incrementing the c variable on every iteration, I would use a while (c >= 0) rather than for.

  • 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.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
  • 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 as const. For example, const int Size = 9; and const 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 and int[] ToCheckPointers should be local variables inside the OutputArrayPoints method, they do not seem to be used outside it.

  • As you are not incrementing the c variable on every iteration, I would use a while (c >= 0) rather than for.

  • 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.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /