Immutable Geometry Types

[I V]

On Sun, 16 Dec 2007 18:13:47 -0800, PeterBraden1 at googlemail.com wrote:
> 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.

I'm not sure it's pythonic to worry too much about enforcing the 
immutability; and disabling setattr makes the internal attribute setting 
a pain in the arse, for little real gain.

> class Angle (object):

I'm not sure about this, but you might want to make Angle a subclass of 
float ; it would save you re-implementing the comparison operators 
(although you'ld still have to re-implement the arithmetic operators to 
get them to return an Angle instance rather than a float).

>     def __init__(self, val, type = 'rad'):
>         if type == 'rad':
>             super(Angle, self).__setattr__('_value', val)
>         else:
>             super(Angle, self).__setattr__('_value',
> math.radians(val))

You might want to normalize the angle to between 0 and 2*pi here (though 
you might not, depending on exactly what you want to represent).

Assuming that the user meant degrees in the else branch strikes me as a 
mistake; what if they mis-typed "rad" as "rsd" or something? Better to 
explicitly check, and raise an exception if the value isn't one of 'rad' 
or 'deg'. Another thing you might want to consider is using keyword 
arguments, rather than a string. Like:

	def __init__(self, radians=None, degrees=None):
		if radians is not None:
			self._value = radians
		elif degrees is not None:
			self._value = math.radians(degrees)
		else:
			raise TypeError("Angle creation must specify \
				keyword argument 'radians' or 'degrees')

Used like:
	a = Angle(math.pi) # Uses radians by default
	a = Angle(radians=2*math.pi) # Or you can specify it explicitly
	a = Angle(degrees=180) # Or specify degrees

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

As I say, I wouldn't bother with these, as they make the implementation 
more annoying to no real gain.

>     def __add__(self, other):
>         if isinstance(other, Angle):
>             return Angle(self._value + other._value)
>         return NotImplemented

Using 'isinstance' is usually a mistake - rather than checking for type, 
you should just use an object as if it were the correct type and, if 
necessary, deal with the resulting exceptions. This means you can use 
objects of a different type, as long as they have the right interface. 
Here, I'd do:

	def __add__(self, other):
		return Angle(self.radians + other.radians)

and likewise for __sub__.
 
>     def __mul__(self, other):
>         """
>         return self * other
>         """
>         if isinstance(other, Angle):
>             return Angle(self._value * other._value)
>         return NotImplemented

Leaving aside my earlier point about not checking for types, does it even 
make sense to multiply an angle by another angle? I would have thought 
you multiplied angles by numbers. So just do:

	def __mul__(self, other):
		return Angle(self._value * other)

And the same for __div__
 

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

You could use decorators here; and the preferred python style for method 
names is all lower case, separated by underscores:

	@classmethod
	def from_cos(self, c):
		return Angle(math.acos(c))

>     radians = property(lambda self: self._value, lambda x:
> self._immutableError())

You don't need to explicitly raise an exception in the setter; just don't 
specify a setter function, and attempting to set the property will raise 
an AttributeError.

> DEG_30 = Angle(math.radians(30))

Given that you've gone to the trouble of creating a constructor that will 
take values as degrees, why not use it here?