HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Ensuring data consistency in a PointsAlongCircle object

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pointsalongcircleconsistencyobjectdataensuring

Problem

I'd like to write classes which are Pythonic, readable and easy-usable. A main issue for me is to keep data consistency. Here is an example (version 1):

from collections import namedtuple
import math

Point = namedtuple('Point', ['x', 'y'])

class PointsAlongCircle(object):
    '''Set of regularly spaced points along a circle'''

    def __init__(self, center, radius, numpoints):
        '''
        center -- center of the circle (Point)
        radius -- radius of the circle (float)
        numpoints -- number of points along the circle

        >>> pts = PointsAlongCircle(Point(0, 0), 3, 4)
        >>> pts.points
        '''
        self.center = center
        self.radius = radius
        self.numpoints = numpoints

        center_x, center_y = self.center

        angles = [i * 2 * math.pi / numpoints for i in range(numpoints)]
        self.points = [Point(center_x + radius*math.cos(angle), center_y + radius*math.sin(angle)) for angle in angles]

pts = PointsAlongCircle(center=Point(0, 0), radius=3, numpoints=4)
print(pts.points)


This code displays as expected

[
    Point(x=3.0, y=0.0), 
    Point(x=1.8369701987210297e-16, y=3.0), 
    Point(x=-3.0, y=3.6739403974420594e-16), 
    Point(x=-5.51091059616309e-16, y=-3.0)
]


This code works. But in Python, public attributes are meant to be played around, right? So someone might legitimately try pts.center = Point(1, 2) and expect that pts.points will be updated to be centered around this new point, which is in reality not the case. And data consistency in the object is broken: the actual center of pts.points is not anymore pts.center.

Also in Python, there is duck-typing. Well, another developer might pass do something like this:

center = [0, 0]
 pts = PointsAlongCircle(center=center, radius=3, numpoints=4) # so far so good
 center[0] = 1. # breaks data consistency in pts


The last line has a side effect: pts.center is updated, and it also breaks the consistency in pts.

He

Solution

To address your questions:

  • Are my assumptions correct?



If you mean the assumption that "public attributes are meant to be played around", then not necessarily. It's pretty well impossible to make things truly private in Python; just being public doesn't really mean "do whatever you like", which brings me to:

  • Is it acceptable to assume that every developer will read the


documentation of the classes I write

No, but it is acceptable to assume that if they don't read the documentation, misuse your code and end up in trouble it's their fault and problem! You will often hear the expression "we're all consenting adults here" in Python - the language has lots of dynamic and introspective features, so we pretty much have to operate on the basis that nobody is particularly trying to do the wrong thing.

  • ...what if I cannot always return read-only versions of the attributes, or if it costs too much resources?



Then that's a trade-off you have to make and document. These issues come up a lot in software development; make the best decision you can at the time, write down why you made it and revisit it if a problem comes up later.

Alternative implementation

All that being said, you're right that if the user can change e.g. center, they will expect the points to be updated accordingly. A third way would be to use caching, trading off storage space against speed. Consider:

class PointsAlongCircle(object):

    _cache = {}  # we will store the sequences of Point objects here

    def __init__(self, center, radius, num_points):
        self.center = Point(*center)  # simpler way to deal with things!
        self.radius = radius
        self.num_points = num_points

    def points(self):
        """Get the points on the circle."""
        key = (self.center, self.radius, self.num_points)
        if key not in self._cache:
            self._cache[key] = self._calculate_points()
        return self._cache[key]

    def _calculate_points(self):
        """The actual calculations happen here, if the result isn't cached."""
        ...


For each combination of center, radius and num_points, the tuple of points is calculated only once.

A few more general review points

This seemed odd:

if isinstance(numpoints, int):  # if the input is an integer
    self._numpoints = int(numpoints)  # convert it to an integer
else:  # otherwise
    raise ValueError('numpoints must be an integer')  # complain about the 'value'?


As an alternative, if you do want to do duck-type checking:

try:
    self._numpoints = int(numpoints)
except ValueError:
    raise TypeError("can't convert {!r} to integer".format(numpoints))


However, note that this will convert float arguments, when the correct behaviour might be to raise an error. Another alternative is to wait for the error to come from range, although if you're using the above method that might be too late!

return tuple(self._points)


This also seemed odd - why not just make self._points a tuple to start with? This will be more efficient, as the current method creates a new object every time the property is called.

Code Snippets

class PointsAlongCircle(object):

    _cache = {}  # we will store the sequences of Point objects here

    def __init__(self, center, radius, num_points):
        self.center = Point(*center)  # simpler way to deal with things!
        self.radius = radius
        self.num_points = num_points

    def points(self):
        """Get the points on the circle."""
        key = (self.center, self.radius, self.num_points)
        if key not in self._cache:
            self._cache[key] = self._calculate_points()
        return self._cache[key]

    def _calculate_points(self):
        """The actual calculations happen here, if the result isn't cached."""
        ...
if isinstance(numpoints, int):  # if the input is an integer
    self._numpoints = int(numpoints)  # convert it to an integer
else:  # otherwise
    raise ValueError('numpoints must be an integer')  # complain about the 'value'?
try:
    self._numpoints = int(numpoints)
except ValueError:
    raise TypeError("can't convert {!r} to integer".format(numpoints))
return tuple(self._points)

Context

StackExchange Code Review Q#92458, answer score: 3

Revisions (0)

No revisions yet.