[Python-il] Code Review: Kakurasu Solver

[Shlomi Fish]

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

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Best Introductory Programming Language - http://shlom.in/intro-lang

<rindolf> She's a hot chick. But she smokes.
<go|dfish> She can smoke as long as she's smokin'.

Please reply to list if it's a mailing list post - http://shlom.in/reply .