On Tue, Nov 9, 2010 at 12:17 AM, Shlomi Fish <[email protected]> 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 <[email protected]> wrote: > > > On Mon, Oct 25, 2010 at 6:49 PM, Shlomi Fish <[email protected]> > 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,
_______________________________________________ Python-il mailing list [email protected] http://hamakor.org.il/cgi-bin/mailman/listinfo/python-il
