current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite

I was recently working on a project where I had the following setup:

from stargaze.sg import SGObject
from stargaze.constants import NegInf, Infinite, PATH_CLAMPED, PATH_REVERSIBLE, PATH_ACCELERATABLE
from stargaze.utils import clamp

__all__ = [
    'PathIdentifier', '_basePath', '_clampedPath', '_reversiblePath', 
    '_acceleratablePath', 'Path'
    ]

class PathIdentifier(SGObject):
    """
    INTERNATL: The base identifier class for a class object.
    """
    OBJ_REPR_PREFIX = "StarGaze Path:: "

class _basePath(PathIdentifier):
    def __init__(self, **config):
        self._time = 0
        self.speed = config['speed']

    def __move__(self):
        self._time += self.speed

class _clampedPath(PathIdentifier):

    def __init__(self, **config):
        start = config['start']
        end = config['end']
        if start > end:
            errmsg = "Start point cannot be greater " \
                     "than the end point."
            raise ValueError(errmsg)
        self._time = start
        self.end = end

        self.finished = False

    def __move__(self):
        if self.finished:
            return

        if self._time >= self.end:
            self.finished = True

class _reversiblePath(PathIdentifier):

    def __init__(self, **config):
        self.direction = 1

    def __move__(self):
        if self.direction == 1 and self._time >= 1:
            self.direction = -1
            self.speed *= -1
        elif self.direction == -1 and self._time <= 0:
            self.direction = 1
            self.speed *= -1

class _acceleratablePath(PathIdentifier):


    def __init__(self, speed, acceleration, min_speed=NegInf, max_speed=Infinite):
    def __init__(self, **config):
        self.accel = config['acceleration']

        self.min_speed = config.get('min_speed', NegInf)
        self.max_speed = config.get('max_speed', Infinite)

    def __move__(self):
        if self.accel != 1:
            self.speed = clamp(self.accel * self.speed, 
                self.max_speed, 
                self.min_speed)

class Path:

    def __init__(self, flags, **config):
        _basePath.__init__(self, **config)
        if flags & PATH_CLAMPED:
            _clampedPath.__init__(self, **config)
        if flags & PATH_REVERSED:
            _reversiblePath.__init__(self, **config)
        if flags & PATH_ACCELERATABLE:
            _acceleratablePath.__init__(self, **config)

        self.flags = flags

    def move(self):
        flags = self.flags

        if flags & PATH_CLAMPED:
            _clampedPath.__move__(self)

        if flags & PATH_REVERSED:
            _reversiblePath.__move__(self)

        if flags & PATH_ACCELERATABLE:
            _acceleratablePath.__move__(self)

None of the code is relevant, you won't have to be able to run any of it to answer my question. What I do in this code essentially is I create classes that define their own __init__ and __move_ methods, but I don't call them from instances of their class. Instead, I call them as class methods acting on the instance of another class. Is this bad style in Python? How else might I achieve what I was trying to do here?

share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

You are using classes as function containers. Path doesn't inherit from any of the other classes, yet you are passing your self instance to their initializer functions.

class _basePath:
    def __init__(self, **config):
        #Initialize Here
class Path:
    def __init__(self, flags, **config):
        _basePath.__init__(self, **config)

is no different from 

def _base_path_init(instance, flag **config):
        #Initialize Here
class Path:
    def __init__(self, flags, **config):
        _base_path_init(self, **config)

You do the same thing in move() to access the specific move implementations.

If you take a step back, this code looks like it is trying to combine the concept of an object model with the concept of mixins or traits. Instead, if we take a step back and focus on the object model concept, the code can be simplified.

class Path(SGObject):
  def __init__(self, flags, **configs):
    SGObject.__init__(self)
    self._traits = []
    if flags & PATH_CLAMPED:
        self._traits.append(ClampedPath(self, **config))
    if flags & PATH_REVERSED:
        self._traits.append(ReversiblePath(self, **config))
    if flags & PATH_ACCELERATABLE:
        self._traits.append(AcceleratablePath(self, **config))

  def move(self):
    for trait in self._traits:
      trait.move(self)

Then each model would have the following interface.

class PathTrait(object):
  def __init__(self, instance, **configs):
     #Initialize Instance Here
  def move(instance):
     #Alter Instance Here

Notes:

The above assumes that the order of move operations needs to be enforced by Path (ie. ClampedPath's move must occur before ReversiblePath's move). If this is not the case, the initializer can be simplified to just take a list of model classes.

class Path(SGObject):
  def __init__(self, traits, **configs):
    SGObject.__init__(self)
    self._traits = [t(self, **config) for t in traits]

This allows you more flexibility in the future and removes the needs for flags entirely.

The way to code is written now, _clampedPath introduces the concept of movement no longer being allowed. If the path is clamped and it has finished, should the other traits execute at all?

share|improve this answer
add comment
up vote 2 down vote

This certainly seems like an odd structure for the code; I've never seen anything implemented like this. I would suggest either:

  1. If you need to have those classes stand-alone (e.g. you also create a _clampedPath() instance directly) you could make them inherited super-classes; or
  2. If you don't, move all of the logic inside Path.

It is not clear which of these cases applies; Python convention is for a leading underscore to indicate "don't use this directly", but then you specifically include those classes in __all__.

The former would mean that the Path methods became:

class Path(ClampedPath, ReversiblePath, AcceleratablePath):

    def __init__(self, flags, **config):
        super().__init__(self, flags, **config)

The flags checking would then be moved to the corresponding methods in the super-classes, e.g.

class AcceleratablePath(BasePath):

    def __init__(self, flags, **config):
        super().__init__(self, flags, **config)
        if flags & PATH_ACCELERATABLE:
            self.accel = config['acceleration']
            self.min_speed = config.get('min_speed', NegInf)
            self.max_speed = config.get('max_speed', Infinite)

In the latter, rather than cram everything into long methods (which may have been why you did this in the first place), split it out like:

class Path(SGObject):

    OBJ_REPR_PREFIX = "StarGaze Path:: "

    def __init__(self, flags, **config):
        super()
        self._time = 0
        self.speed = config['speed']
        if flags & PATH_ACCELERATABLE:
            self._init_accel(flags, **config)
        ...

    def _init_accel(self, flags, **config):
        self.accel = config['acceleration']
        self.min_speed = config.get('min_speed', NegInf)
        self.max_speed = config.get('max_speed', Infinite)

Also, in terms of following Python conventions, the method name format __foo__ is reserved for special method names; you should call your method(s) move. Per PEP-0008 (emphasis mine):

__double_leading_and_trailing_underscore__: "magic" objects or attributes that live in user-controlled namespaces. E.g. __init__, __import__ or __file__. Never invent such names; only use them as documented.

share|improve this answer
add comment

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.