[Python-il] Code Review: Kakurasu Solver

cool-RR 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>
> wrote:
> > >> > 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
> python?
> > >
> > > Here's a piece by Guido:
> > > http://python-history.blogspot.com/2010/06/new-style-classes.html
> > >
> > > And a StackOverflow question:
> > >
> > >
> http://stackoverflow.com/questions/54867/old-style-and-new-style-classes-
> > > 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
> (plug
> > 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
> an
> > 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
> minimal
> 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
> Params
> > 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:
>
> http://bitbucket.org/shlomif/lp-based-puzzle-solvers
>
> 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
> figure
> 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
> now.
>
> > (and BTW, why the "_" prefix in _print_sol? After all, if you call it
> from
> > 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?
>
> Regards,
>
>        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_horiz_constraints,
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
solution.        '''


That's the end of my review-- Since I don't have time to actually
understand the algorithm.



Ram,
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://hamakor.org.il/pipermail/python-il/attachments/20101109/2aac101a/attachment-0001.htm 


More information about the Python-il mailing list