[Python-il] Code Review: Kakurasu Solver
cool-rr at cool-rr.com
Tue Nov 9 00:52:40 IST 2010
On Tue, Nov 9, 2010 at 12:17 AM, Shlomi Fish <shlomif at iglu.org.il> wrote:
> Hi Amit,
> On Tuesday 26 October 2010 01:46:29 Amit Aronovitch wrote:
> > On Tue, Oct 26, 2010 at 12:26 AM, cool-RR <cool-rr at cool-rr.com> wrote:
> > > On Mon, Oct 25, 2010 at 6:49 PM, Shlomi Fish <shlomif at iglu.org.il>
> > >> > I have a few general notes:
> > >> > 1. Use new-style classes. (Inherit every class from `object`.)
> > >>
> > >> What does that give me? Is it compatible with older versions of
> > >
> > > Here's a piece by Guido:
> > > http://python-history.blogspot.com/2010/06/new-style-classes.html
> > >
> > > And a StackOverflow question:
> > >
> > >
> > > in-python
> > >
> > > Old-style classes are deprecated and you should never use them.
> > I actually wanted to say that you can't use descriptors on old-styles
> > for my own ravings http://alturl.com/kiz2r , but for me, this would
> > actually be a practical reason in favor on new-styles, since I do use
> > @cprop quite often)...
> > But then I saw shlomif using @staticmethod, which I thought should cause
> > error in his program - and apparently it *does work*!
> > A little test shows that even user defined descriptors do get called with
> > old classes...
> > Checking the docs:
> > http://docs.python.org/reference/datamodel.html#descriptors says:
> > "Note that descriptors are only invoked for new style objects or classes
> > (ones that subclass
> > object()<http://docs.python.org/library/functions.html#object>or
> > type() <http://docs.python.org/library/functions.html#type>)."
> > WTF?
> > Well, it turns out that it is not the object (the owner of the attribute)
> > that has to be new-style. It's the descriptor (the attribute - e.g.
> > property/staticmethod/etc.) itself that must be new.
> > Makes some sense, but the docs *are* misleading (the word "for" is a bad
> > choice, should be something like "if they are" instead), and I think the
> > StackOverflow posters and probably others were misled by it, as I was.
> > I also like using type(obj) rather than obj.__class__ (and maybe some
> > debuggers/IDE's/autodoc-systems make use of such interfaces and would
> > sooner or later deprecate support for old-style classes).
> > > About compatibility: I think new-style classes go back to Python 2.2.
> > True. But there *is* ancient code out there (though personally I prefer
> > future compatibility if I have a choice)
> > For old Pythons, adding "class object: pass" would probably make it work
> > (but then you might have other problems, such as iterators, nested scopes
> > etc. that just do not work in 2.1).
> Interesting discussion that. I don't care too much about compatibility with
> Python-2.1 and below, because they are very old. Not sure what is the
> version of Python that the lp_solve bindings work with anyhow.
> > <--- some style-related comments snipped --->
> > I also did not look too deep into the code, but I did notice that
> > _process_constraints does not use self at all,
> I moved _process_constrains to Params as you suggested below.
> > and its caller
> > _calc_params_obj only uses it to copy some attributes. This probably
> > indicates some OO design issue (probably these methods belong in the
> > class, possibly in/called by a constructor) - I'm sure with a little
> > rethinking you can make your code shorter and more readable.
> I moved them to the Params class, thanks:
> I'm now thinking that maybe I should extract the duplicate logic of the
> horizontal and vertical constraints into a common class. But I couldn't
> out a good way to do that back when I wrote it.
> > I also think that instead of the last three or so _print_x / _output_x
> > functions, you could do something like
> > for row in sol:
> > print "".join(map(_cell_as_text, row))
> > (or you could make it a three-liner and a bit more readable, still faster
> > to understand than tracking functionality down through three function
> > calls IMO)
> Yes, this was the case of over-refactoring. I changed it to your version
> > (and BTW, why the "_" prefix in _print_sol? After all, if you call it
> > your main, you probably do not want to make it private).
> Don't know, I didn't want to expose it. I now changed it to
> print_sol_to_screen_as_unicode without the prefix.
> Does anyone have any more comments?
> Shlomi Fish
Just one more doc-style comment:
def solve(self): ''' This method attempts to solve the
game after self.width , self.height ,
self.num_known_horiz_constraints , self.num_known_vert_constraints ,
self.horiz_constraints and self.vert_constraints were filled in.
It returns a two-dimensional array containing the rows of the
boolean values with the solution. '''
Here's how this comment should look:
def solve(self): '''
Attempt to solve the game.
This should be called after self.width, self.height,
self.num_known_vert_constraints, self.horiz_constraints and
self.vert_constraints were filled in. Returns a two-dimensional
array containing the rows of the boolean values with the
That's the end of my review-- Since I don't have time to actually
understand the algorithm.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Python-il