0
\$\begingroup\$

I wrote the Python3 code below. This is actually a stripped down version of a bigger project, but in essence it boils down to this code. There are two classes: Point, which is a point in 3D space and PointCloud, which is a sequence (list) of points.

In the PointCloud class, I have a method to calculate the minimum and maximum values for each axis: minmax_coords, which returns a dict (keys: the axes 'x', 'y', 'z') of a dict (keys: 'min' and 'max'). So you can do: somepointcloud.minmax_coords()['x']['min'] for the minimal x-axis value.

As it is, the code doesn't feel very pythonic and isn't really self-explanatory.

My question is: please review the code of the minmax_coord method and give me some suggestions how to refactor the code to make it more pythonic and more self explanatory.

Any suggestions to improve the code are welcome.

Repl link to the code.

class Point:
    """ Point with coordinate """
    def __init__(self, coordinate=None):
        """ Constructor """
        self.coordinate = coordinate


class PointCloud:
    """ A cloud (list) of Points """
    def __init__(self, point_list=None):
        """ Constructor """
        self.points = point_list

    def minmax_coords(self):
        """ Return minimum and mainum coordinate for each axis """
        if not self.points:
            raise IndexError("PointCloud has no Points.")

        result = {'x': {}, 'y': {}, 'z': {}}
        for i, axis in zip(range(3), ['x', 'y', 'z']):
            result[axis]['min'] = self.points[0].coordinate[i]
            result[axis]['max'] = self.points[0].coordinate[i]

        for point in self.points:
            for i, axis in zip(range(3), ['x', 'y', 'z']):
                result[axis]['min'] = min(result[axis]['min'],
                                          point.coordinate[i])
                result[axis]['max'] = max(result[axis]['max'],
                                          point.coordinate[i])

        return result


if __name__ == "__main__":
    P = PointCloud([Point((1, 2, 3)), Point((5, 4, 3))])
    print(f"Minimum and maximum coordinates: {P.minmax_coords()}")
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Is there anything else in these classes? It's hard to advise you when you say that it's stripped down, but we don't know what is omitted. Based on the little bit that you've shown, I'd say that you shouldn't bother with writing these as classes at all. \$\endgroup\$ – 200_success Oct 7 '18 at 17:44
1
\$\begingroup\$

As @200_success mentions, these classes seem a bit sparse, and you shouldn't bother with classes for them in the first place.

For instance, Point could be nicely replaced with a namedtuple.

>>> from collections import namedtuple
>>> Point = namedtuple('Point', ['x', 'y', 'z'], defaults=[0,0,0])
>>> p1 = Point(1,4,3)
>>> p2 = Point(5,2,3)

The components of Point can now easily be referred to by name (eg, p1.x or p2.z) and can still be referred to by index (eg, p1[0] or p2[2]).

Similarly, you don't need a whole class just for a minmax_coords function:

>>> def minmax_coords(*pnts):
...    mn = Point( *(min(p[i] for p in pnts) for i in range(3)) )
...    mx = Point( *(max(p[i] for p in pnts) for i in range(3)) )
...    return mn, mx
... 
>>> minmax_coords(p1, p2)
(Point(x=1, y=2, z=3), Point(x=5, y=4, z=3))
>>> 

Note: As opposed to a dictionary of dictionaries for the result, I've changed the return into two points: a point with the minimum coordinate on each axis, and a point with the maximum coordinate on each axis.

Demystifying the minmax_coords implementation:

  • min(p[i] for p in pnts) computes the minimum value of the i'th axis over all of the points.
  • _____ for i in range(3) is a generator expression which executes that minimum computation for each axis i=0, i=1 and i=2.
  • Point( *(____) ) immediately evaluates the generator expression and uses the values as the 0th, 1st, and 2nd arguments of the Point() constructor.
\$\endgroup\$

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