Immutable Geometry Types

[Gabriel Genellina]

En Sun, 16 Dec 2007 23:13:47 -0300, PeterBraden1 at googlemail.com  
<PeterBraden1 at googlemail.com> escribi�:

> Hi,
>
> I am learning python, having learnt most of my object orientation with
> java, and decided to port some of my geometry classes over. I haven't
> used immutability in python before, so thought this would be an
> interesting chance to learn.
>
> I am looking for feedback on any ways in which I might have left
> variables unprotected, and ways in which I am not being pythonic.

Immutable classes usually implement __new__ and leave __init__  
unimplemented:

     def __new__(cls, val, measure='rad'):
         # using 'type' as a name shadows the builtin type
         instance = super(Angle, self).__new__()
         if measure == 'rad': pass
         elif measure == 'deg': value = math.radians(value)
         else: raise ValueError, "unknown Angle measure: %r" % measure
         instance._value = value
         return instance

For the various comparison operators, since this class "plays well" with  
ordering, it's easier to implement __cmp__ and make all operators refer to  
it:

     def __cmp__(self, other):
        if isinstance(other, Angle):
           return cmp(self._value, other._value)
        raise NotImplementedError

     __eq__ = lambda self,other: self.__cmp__(other)==0
     __ne__ = lambda self,other: self.__cmp__(other)!=0
     __lt__ = lambda self,other: self.__cmp__(other)<0
     __ge__ = lambda self,other: self.__cmp__(other)>=0
     __gt__ = lambda self,other: self.__cmp__(other)>0
     __le__ = lambda self,other: self.__cmp__(other)<=0

>     def __setattr__(self, name, value):
>         """
>         Suppress setting of data - Angle is immutable
>         """
>         self._immutableError()

Why? This class contains a single attribute, value, and we could make it  
immutable. But why restrict the possibility to add *other* attributes?
(There is __slots__ too, but I would not recommend it at this stage)

>     def __delattr__(self, name):

Same as above.

>     def __mul__(self, other):
>     def __div__(self, other):

(I've never seen those operations on angles)

>     def fromCos(self, c):
>         """
>         return angle with specified cos
>         """
>         return Angle(math.acos(c))
>
>     fromCos = classmethod(fromCos)

I prefer to write

     @classmethod
     def fromCos(self, c):

>     radians = property(lambda self: self._value, lambda x:
> self._immutableError())
>     degrees = property(lambda self: math.degrees(self._value), lambda
> x: self._immutableError())
>     cos = property(lambda self: math.cos(self._value), lambda x:
> self._immutableError())
>     sin = property(lambda self: math.sin(self._value), lambda x:
> self._immutableError())
>     tan = property(lambda self: math.tan(self._value), lambda x:
> self._immutableError())

You don't have to write them that way. Just omit the property setter, and  
it will be read-only.

     tan = property(lambda self: math.tan(self._value))

Or:

     @property
     def tan(self):
         return math.tan(self.value)

>     def withinRange(self, angle, range):
>         """
>         angle is within range of self
>         """
>         return (self._value < angle._value + range) and (self._value >
> angle._value - range)

I think this is more readable:

	return angle.value-range < self.value < angle.value+range

> class Point2D (object):

Same as above, replace __init__ by __new__. You may inherit from tuple  
instead, and get some basic methods already implemented.

>     length = property(lambda self: return math.sqrt(self.x*self.x +
> self.y*self.y), lambda x: self._immutableError())

Using math.hypot is safer in some circunstances.

>     def normalise(self):
>         return Point2D(self.x/self.length, self.y/self.length);
>     def translate(self, other) {
>         return self + other
>     }

I'd use "normalised", "translated", etc. because they return a *new*  
object, instead of modifying self.
(You forgot to remove some braces, I presume...)

-- 
Gabriel Genellina