Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Alternative implementation

Alternative implementation

###A few more general review points

A few more general review points

###Alternative implementation

###A few more general review points

Alternative implementation

A few more general review points

Source Link
jonrsharpe
  • 14k
  • 2
  • 36
  • 62

To address your questions:

  1. 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:

  1. 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.

  1. ...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.

lang-py

AltStyle によって変換されたページ (->オリジナル) /