Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm a very newbie coder working with Python; I wanted some tips, general criticism/help, and advice concerning my coding patterns with the architecture I'm implementing (using linear algebra as my basis). Everything I'm building will rely on the following code, so I want to make sure I'm not shooting myself in the foot, and that my design patterns are reasonable and flexible.

One very specific change I'd like to make: I'd like to allow for equality tests between points and vectors with an unequal number of components by assuming that all undefined components are zero (all points and vectors have infinite components; equality tests stop testing once a check for both components return 'undefined'). Mostly I'd like to do this just because it seems kind of cool to do, so it is not remarkably important, but if someone has a suggestion how I can get there (or wants to tell me why that is a stupid idea), I'd love to hear more.

But in essence, I really just want someone to rip this code apart and tell me why I'm doing everything wrong (or tell me that I'm doing things pretty alright). The goal is just to have a nice, sane Point and Vector system that is very flexible and can be understood by anyone--and used by anyone--for any purpose they'd like.

Thanks for taking the time to read!

import math, sys, os

TOLERANCE = 0.00001

class SizeError(Exception):
    def __init__(self, *args):
        self.args = args
        print "Size Error between " + str(args)

class P(object):
    def __init__(self, *args):
        if len(args) == 1:
            try:
                self.components = tuple(args[0])
                self.size = len(args[0])
            except TypeError:
                self.components = tuple(args)
                self.size = len(args)
        else:
            self.components = args
            self.size = len(args)
    def __getitem__(self, key):
        return self.components[key]
    def __repr__(self):
        result = ""
        for i in range(0, self.size):
            result += str(self[i]) + ", "
        return "<Point (" + str(self.size) + ") : "  + result[:-2] + ">"
    def __eq__(self, other):
        if self.size != other.size:
            raise SizeError(self, other)
        for i in range(0, self.size):
            result = self[i] - other[i]
            if math.fabs(result) > TOLERANCE:
                return False
        return True
    def __add__(self, other):
        if other.size != self.size:
            raise SizeError(self, other)
        result = []
        for i in range(0, self.size):
            result.append(self[i] + other[i])
        return P(result)
    def __sub__(self, other):
        if other.size != self.size:
            raise SizeError(self, other)
        result = []
        for i in range(0, self.size):
            result.append(self[i] - other[i])
        return V(result)

class V(P):
    def __repr__(self):
        result = ""
        for i in range(0, self.size):
            result += str(self[i]) + ", "
        return "<Vector (" + str(self.size) + ") : "  + result[:-2] + ">"
    def __add__(self, other):
        if other.size != self.size:
            raise SizeError(self, other)
        result = []
        for i in range(0, self.size):
            result.append(self[i] + other[i])
        return V(result)
    def __mul__(self, scalar):
        result = []
        for i in range(0, self.size):
            result.append(self[i] * scalar)
        return V(result)
    def __neg__(self):
        return self * -1
    def dotProduct(self, other):
        result = 0
        if self.size != other.size:
            raise SizeError(self, other)
        for i in range(0, self.size):
            result += self[i] * other[i]
        return result
    def normalize(self):
        result = []
        length = self.getLength()
        if math.fabs(length) < TOLERANCE: return self.getZero()
        for i in range(0, self.size):
            result.append(self[i] / length)
        return V(result)
    def getLength(self):
        result = 0
        for i in range(0, self.size):
            result += self[i]**2
        return math.sqrt(result)
    def getZero(self):
        return self * 0
    def isZero(self):
        zero = self.getZero()
        return self == zero
share|improve this question
    
Why don't you use numpy? It's fast, reliable and extremely powerful. –  P3trus Nov 9 '12 at 20:29
    
To be honest, I expect that that's precisely what I'll eventually end up doing. But I want to make sure I understand good architecture before I start relying on other people's excellent architecture. –  user19115 Nov 9 '12 at 20:46
    
You got good feedback. My 2 cents - I don't see how vector 'is-a' point - the hierarchy doesn't make sense. –  avip Nov 10 '12 at 7:24

1 Answer 1

up vote 3 down vote accepted
import math, sys, os

TOLERANCE = 0.00001

class SizeError(Exception):
    def __init__(self, *args):
        self.args = args
        print "Size Error between " + str(args)

You shouldn't print in an exception constructor. At most, you should define a __repr__ for printing purposes. But you can probably get away with not doing that and trusting the default.

class P(object):

The name doesn't give me much clue as to what this class is.

    def __init__(self, *args):
        if len(args) == 1:
            try:
                self.components = tuple(args[0])
                self.size = len(args[0])
            except TypeError:
                self.components = tuple(args)
                self.size = len(args)
        else:
            self.components = args
            self.size = len(args)

Instead of assigning size three times, use self.size = len(self.components) at the end. In fact, consider not storing the size at all and just looking at the size of the components. I'd also suggest not accepting three different ways of passing in the parameters. Pick one and make it be used everywhere.

    def __getitem__(self, key):
        return self.components[key]
    def __repr__(self):
        result = ""
        for i in range(0, self.size):
            result += str(self[i]) + ", "

Add strings is expensive. But python has a really nice join function. So you could do:

result = ",".join(map(str,self))

And it'll actually take care of everything else for you

        return "<Point (" + str(self.size) + ") : "  + result[:-2] + ">"
    def __eq__(self, other):
        if self.size != other.size:
            raise SizeError(self, other)
        for i in range(0, self.size):

You don't need the zero.

            result = self[i] - other[i]
            if math.fabs(result) > TOLERANCE:

I'd combine those two lines

                return False
        return True
    def __add__(self, other):
        if other.size != self.size:
            raise SizeError(self, other)
        result = []
        for i in range(0, self.size):
            result.append(self[i] + other[i])

Use zip, and it'll be a bit faster to use a list comprehension:

result = [x+y for x, y in zip(self, other)]

        return P(result)
    def __sub__(self, other):
        if other.size != self.size:
            raise SizeError(self, other)
        result = []
        for i in range(0, self.size):
            result.append(self[i] - other[i])
        return V(result)

class V(P):

I wouldn't have vector inherit from point. I'd have them both inherit from some other more generic class. A vector is not a point, coding like it is one could be confusing.

    def dotProduct(self, other):

Python convention is to have names which are lowercase_with_underscores for methods.

        result = 0
        if self.size != other.size:
            raise SizeError(self, other)
        for i in range(0, self.size):
            result += self[i] * other[i]
        return result

Here I'd use

return sum(x * y for x, y in zip(self, other))


    def normalize(self):
        result = []
        length = self.getLength()
        if math.fabs(length) < TOLERANCE: return self.getZero()
        for i in range(0, self.size):
            result.append(self[i] / length)
        return V(result)
    def getLength(self):
        result = 0
        for i in range(0, self.size):
            result += self[i]**2
        return math.sqrt(result)

    def getZero(self):
        return self * 0
    def isZero(self):
        zero = self.getZero()
        return self == zero

Both of these will be inefficent implementations. They should work, but they'll be relatively slow. The fact being any Point/Vector class implemented in python will be unforgivably slow. That's why we typically implement such things in extensions like numpy.

share|improve this answer
    
Thank you tremendously for your help! –  user19115 Nov 10 '12 at 0:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.