GNOME Bugzilla – Bug 578903
clean up sudoku code
Last modified: 2013-11-07 18:15:48 UTC
The current sudoku code needs some cleanup, for example, no spaces around operators, useless import, unused but not-deleted code. With code checking tools like pychecker, pyflakes and pylint, it wouldn't be difficult to spot the problems. Mark as gnome-love.
We can also keep a pylintrc in the repository, like what orca does, http://svn.gnome.org/viewvc/orca/trunk/pylintrc. There is also a script to run pylint, http://svn.gnome.org/viewvc/orca/trunk/run_pylint.sh.in.
Created attachment 132669 [details] pylintrc file My pylintrc file for gnome-sudoku Increases line length to 120 Ignores missing docstring Accepts x and y as variable names Prints msg-id with the line number/desribtion The list of msg-id's above "disble-msg" is copied from orca's pylintrc file.
Created attachment 132935 [details] [review] main.py: comma followed by space Fixing :C0324: *Comma not followed by a space* Use this regex in VIM: :%s/[^ \t$]\@=,[^ \t$]\@=/, /gc i.e., if a comma is preceded by a character (which is not space/tab/end-of-line), and followed by a character (which is not space/tab/end-of-line), then add a space before it. IIRC, there are 174 such cases. But two of them is not changed, which is inside a comment. So that's 172 of them.
Created attachment 132936 [details] [review] gsudoku.py: delete useless arguments to a callback The key_press_cb I introduced days before doesn't need that parameters. So delete them, my fault. BTW, I mean 'add a space *after* the comma' in comment #3. And, although the regex is simple, that patch is lengthy. So you may just apply the regex instead of the patch.
Created attachment 132939 [details] [review] main.py: operator not preceded by space "C0322 Operator not preceded by a space" is gone. This patch is generated _after_ attachment #132935 [details], not against current master. Or should I always diff against master?
Looks good. It is okay to assume that the first patch is already applied in this case. Diff works on a line by line granularity so two patches against master for this would end in a ton of conflicts. I will commit this when I get home. Much cleaner now :)
Created attachment 132950 [details] [review] loop variable used outside of loop one line patch :) gsudoku.py, :W0631: *Using possibly undefined loop variable %r*
Patches commited to trunk: http://git.gnome.org/cgit/gnome-games/commit/?id=ec67935b4cfd3f8d7974a129437569ad708d54db Thanks!
New scores for all files i src/lib: colors.py -2.44 dancer.py 10.00 dialog_swallower.py 4.29 game_selector.py 3.86 gnome_sudoku.py 8.57 gsudoku.py 7.27 main.py 7.07 pausable.py 4.17 printing.py 5.20 saver.py 6.23 simple_debug.py 0.00 sudoku_generator_gui.py 0.40 sudoku_maker.py 4.53 sudoku.py 5.23 sudoku_thumber.py 4.12 timer.py 9.89
Created attachment 133111 [details] [review] delete unused methods in sudoku.py These methods are not used, according to grep. A tool to find out unused variables/functions would be helpful...
Created attachment 133486 [details] [review] Refine the parameters of complain_conficts use coordinates, not widget object, as parameters. also add docstring.
Created attachment 133487 [details] [review] Extract a method to remove error highlights 'Extract Method' on the code to remove error highlights. The original code has, # Its possible this highlighted error was never # added to our internal grid, in which case we'd # better make sure it is... I think this is not necessary, so delete it. Please add it back if not proper.
Committed to master. Thanks Zhang!
Created attachment 134294 [details] [review] eliminate most wildcard import Define '__all__' in defaults.py.in, and eliminate most 'from defaults import *'. But main.py uses too many from defaults, so keep the import there. This patch is built against the patch in bug 571475.
Committed with a little change, http://git.gnome.org/cgit/gnome-games/commit/?id=5ce42c0d2069305fd0d62b62c373d693d1f825ae
/me is about to do a big commit and cross fingers... It's all about adding spaces and newlines. Here is what it smells like now, 3.13 : sudoku_generator_gui 4.29 : dialog_swallower 4.55 : simple_debug 5.34 : colors 5.41 : sudoku_thumber 6.77 : number_box 6.88 : pausable 6.89 : sudoku_maker 7.54 : game_selector 7.64 : gsudoku 7.91 : printing 8.03 : saver 8.16 : sudoku 8.57 : main 9.00 : defaults 9.33 : gnome_sudoku 9.89 : timer 10.00 : dancer
Created attachment 161840 [details] [review] Refactored long lines in main.py First commit attempt. If it's okay, I'll fix some more pylint warnings.
Review of attachment 161840 [details] [review]: The changes have added newlines into the strings, i.e. """ abc def """ is the same as "\nabc\ndef\n" not "abc\ndef" which would be: """abc def"""
Created attachment 163070 [details] [review] Fixed accidental newlines from patch in attachment 161840 [details] [review]
Again, the strings have been modified, for example: """abc def""" == "abc\n def", not "abc\ndef" If you have a long string the only solution is to split it with backslashes, e.g. "This is a long\ string that I didn't\ want in one line." You can move substitutions with a backslash too, e.g. "Put the number one in here: %d" % a_long_variable_name can be "Put the number one in here: %d" \ % a_long_variable_name (also please create a new patch instead of patching a patch, thanks!)