HW3.8 / Sudoku: Please check my code

I don't quite get why the code I submitted was wrong. It works with the sample sudokus mentioned in the exercise, and it worked with the test cases I came up with. Feedback is appreciated!

Thank you!

Here's the code:

correct = [[1,2,3],
           [2,3,1],
           [3,1,2]]

incorrect = [[1,2,3,4],
             [2,3,1,3],
             [3,1,2,3],
             [4,4,4,4]]

rows = []
columns = []

def check_rows(sudoku):
    for i in range(0, len(sudoku)):
        rows.append(set((sudoku[i])) == set(range(1, len(sudoku)+1)))

    if False in rows:
        return False
    else:
        return True

def check_columns(sudoku):
    zipped = zip(*sudoku)
    for i in range(0, len(zipped)):
        columns.append(set((zipped[i])) == set(range(1, len(zipped)+1)))

    if False in columns:
        return False
    else:
        return True

def check_sudoku(sudoku):
    return check_rows(sudoku) and check_columns(sudoku)

===============

You can check the results yourself with either of those two commands:
print check_sudoku(incorrect)
print check_sudoku(correct)

asked 18 Mar '12, 14:55

Gregor%20Ulm's gravatar image

Gregor Ulm
51361635
accept rate: 47%

edited 18 Mar '12, 14:55

Thanks for sharing your code, it's a nice variation on what I've seen before.

(18 Mar '12, 16:21) fnenu-1 ♦♦ fnenu-1's gravatar image

3 Answers:

I think it doesn't run when it's called from the grader since you have rows = [] and columns = [] defined outside of the procedures. I don't think anything which is not contained in a procedure is used.

If I try to run the code multiple times, since rows = [] and columns = [] doesn't get reset, it gives very mixed results. For instance, if I run
[[4, 3, 2, 1], [3, 2, 1, 4], [2, 1, 4, 3], [1, 4, 3, 2]] in isolation, your code returns True but if I run it after some others, I get False since on the second run through, rows and columns are equal to what they were at the end of the last run through.

Edit: Putting columns = [] under def check_columns(sudoku): and rows = [] under def check_rows(sudoku): should fix it.
@Angel's suggestion streamlines your code nicely (as having duplicate code is generally not a good idea as if there is an error, you have to change it twice).

link

answered 18 Mar '12, 16:15

fnenu-1's gravatar image

fnenu-1 ♦♦
18.5k1981231

edited 18 Mar '12, 16:20

LOL I was mesmerized by the duplication, didn't even notice the lists at the top o.O
Told you, too much sudoku.. didn't run the code in my tests, that's why!

(18 Mar '12, 16:23) Angel Angel's gravatar image

And I completely missed the duplication!

(18 Mar '12, 16:24) fnenu-1 ♦♦ fnenu-1's gravatar image

Thanks for the feedback. I didn't know how the grader actually works and assumed the procedure gets just called once and then restarted (at least that's how I tested my code in an online Python shell). Well, you live and learn.

(19 Mar '12, 12:30) Gregor Ulm Gregor%20Ulm's gravatar image

I think that what happened is also something that could happen if you make the code to fit in with someone else's code so it's a good thing to find out about now when it's just a course rather than if you were coding something important later in life and the same sort of bug arose because the variables weren't set to zero for each example run through the code.

(19 Mar '12, 23:13) fnenu-1 ♦♦ fnenu-1's gravatar image

I think you have your first example on the perils of global variables.

After you run check_sudoku on an incorrect sudoku, since then, the following calls to check_sudoku will return False though it is correct or not. For example:

print check_sudoku(correct)
print rows
print check_sudoku(incorrect)
print rows
print check_sudoku(correct)
print rows

You get:

True
[True, True, True]
False
[True, True, True, True, False, False, False]
False
[True, True, True, True, False, False, False, True, True, True]

Ups, the second time you call check_sudoku(correct) you get False. Why? Because of the global variable row that you did not reset to [] before append new elements. Your row variable contains all the elements from previous function calls.

My humble suggestion:

Try to avoid global variables. You could have those variables as local variables inside each method instead.

link

answered 18 Mar '12, 16:32

dreyescat's gravatar image

dreyescat
6.9k163482

Ups, @fnenu answered faster... So, as you can see I completely agree with him ;)

(18 Mar '12, 16:36) dreyescat dreyescat's gravatar image

I'm not saying it's a bug, but... suggestion here:

def check_sudoku(sudoku):
return check_rows(sudoku) and check_rows(zip(*sudoku))

link

answered 18 Mar '12, 16:05

Angel's gravatar image

Angel
7.0k632112

Your answer
Question text:

Markdown Basics

  • *italic* or _italic_
  • **bold** or __bold__
  • link:[text](http://url.com/ "Title")
  • image?![alt text](/path/img.jpg "Title")
  • numbered list: 1. Foo 2. Bar
  • to add a line break simply add two spaces to where you would like the new line to be.
  • basic HTML tags are also supported

Tags:

×15,257
×323
×118
×103
×23

Asked: 18 Mar '12, 14:55

Seen: 316 times

Last updated: 19 Mar '12, 23:13