Is this use of isinstance pythonic/"good"?

Question

A side effect of this question is that I was lead to this post, which states:

Whenever isinstance is used, control flow forks; one type of object goes down one code path, and other types of object go down the other --- even if they implement the same interface!

and suggests that this is a bad thing.

However, I've used code like this before, in what I thought was an OO way. Something like the following:

class MyTime(object):
    def __init__(self, h=0, m=0, s=0):
        self.h = 0
        self.m = 0
        self.s = 0
    def __iadd__(self, other):
        if isinstance(other, MyTime):
            self.h += other.h
            self.m += other.m
            self.s += other.s
        elif isintance(other, int):
            self.h += other/3600
            other %= 3600
            self.m += other/60
            other %= 60
            self.s += other
        else:
            raise TypeError('Addition not supported for ' + type(other).__name__)

So my question:

Is this use of isinstance "pythonic" and "good" OOP?

Answer 1

The first use is fine, the second is not. Pass the argument to int() instead so that you can use number-like types.

Answer 2

Not in general. An object's interface should define its behavior. In your example above, it would be better if other used a consistent interface:

def __iadd__(self, other):
    self.h += other.h
    self.m += other.m
    self.s += other.s

Even though this looks like it is less functional, conceptually it is much cleaner. Now you leave it to the language to throw an exception if other does not match the interface. You can solve the problem of adding int times by - for example - creating a MyTime "constructor" using the integer's "interface". This keeps the code cleaner and leaves fewer surprises for the next guy.

Others may disagree, but I feel there may be a place for isinstance if you are using reflection in special cases such as when implementing a plugin architecture.

Answer 3

isinstance, since Python 2.6, has become quite nice as long as you follow the "key rule of good design" as explained in the classic "gang of 4" book: design to an interface, not to an implementation. Specifically, 2.6's new Abstract Base Classes are the only things you should be using for isinstance and issubclass checks, not concrete "implementation" types.

Unfortunately there is no abstract class in 2.6's standard library to summarize the concept of "this number is Integral", but you can make one such ABC by checking whether the class has a special method __index__ (don't use __int__, which is also supplied by such definitely non-integral classes as float and str -- __index__ was introduced specifically to assert "instances of this class can be made into integers with no loss of important information") and use isinstance on that "interface" (abstract base class) rather than the specific implementation int, which is way too restrictive.

You could also make an ABC summarizing the concept of "having m, h and s attributes" (might be useful to accept attribute synonyms so as to tolerate a datetime.time or maybe timedelta instance, for example -- not sure whether you're representing an instant or a lapse of time with your MyTime class, the name suggests the former but the existence of addition suggests the latter), again to avoid the very restrictive implications of isinstance with a concrete implementation cass.

Answer 4

To elaborate further on the comment I made under Justin's answer, I would keep his code for __iadd__ (i.e., so MyTime objects can only be added to other MyTime objects) and rewrite __init__ in this way:

def __init__(self, **params):
    if params.get('sec'):
        t = params['sec']
        self.h = t/3600
        t %= 3600
        self.m = t/60
        t %= 60
        self.s = t
    elif params.get('time'):
        t = params['time']
        self.h = t.h
        self.m = t.m
        self.s = t.s
    else:
        if params:
            raise TypeError("__init__() got unexpected keyword argument '%s'" % params.keys()[0])
        else:
            raise TypeError("__init__() expected keyword argument 'sec' or 'time'")

# example usage
t1 = MyTime(sec=30)
t2 = MyTime(sec=60)
t2 += t1 
t3 = MyTime(time=t1)

I just tried to pick short keyword arguments, but you may want to get more descriptive than I did.