13

I've got a 2D array of different blocks, all inheriting from Block. I want to check if the block that I clicked on is a Dirt type block, like this:

clickedblock = getClickedBlock()

if isinstance(clickedblock, Dirt):
    place a block

else:
    don't place a block

I've heard that isinstance is bad, and should be avoided because it creates forks in code. What times would isinstance be good to use?

Another more cumbersome solution for my problem would be to have a field of Block called 'id' and then check if it equals some constant that means Dirt. But that sounds quite bad and more prone for mistake than the simple isinstance.

6
  • @DietrichEpp Which is why I'm asking here for confirmation.
    – SuperDisk
    Nov 29, 2012 at 22:49
  • @Keyser: That's exactly my point. You disagree, but you don't tell me why you disagree, so you don't give other people the chance to make up their own minds if they so choose. Nov 29, 2012 at 22:51
  • @DietrichEpp Me not stating my reasons doesn't prevent people from making up their own minds. Perhaps even the opposite. The reason I didn't was because it's not the right forum :)
    – keyser
    Nov 29, 2012 at 22:52
  • 3
    I suppose the most object oriented way would be something like if clickedblock.is_placeable(position): ... or try: clickedblock.place(position) Nov 29, 2012 at 22:53
  • @DietrichEpp the beginning of your statement is a bit condescending. For all you know, they could be a child (in fact it looks like OP was around 14 when the question was asked). It also implies that asking such a question is childish (it's not). BTW, whether someone tells you if something is good or bad is besides the point. Programmers are interested in knowing when isinstance is justified and when it should be avoided.
    – Dennis
    Nov 5, 2014 at 0:31

4 Answers 4

14

Your example seems like a legitimate use case of isinstance().

It's not that isinstance() is bad, often polymorphism can be used for the same purpose (which results in cleaner code in where the class is used).

But sometimes, isinstance() is what you need. For example, the pythonic way of detecting whether a variable is string or not is isinstance(var, basestring).

3

I learned the hard way against using it. The problem is that the outcome is sensitive to how the class definitions were imported:

  • in the place where the object was instantiated
  • in the place where the isinstance test is performed

If one import was relative and the other absolute - the check will fail. Essentially it'll be like checking on equality between SomeClass vs somepackage.SomeClass. It doesn't even matter that they come from the same file and so on. Also, there will be a similar outcome if you somehow have both the root directory and somepackage directory in your PYTHONPATH - then an absolute-styled import could denote a path from either of the "source roots" so two different absolute-styled imports would result in failing instance checks.

One could argue about how good practices would anyway prevent that from happening but good practices are also largely about not taking chances. In that spirit I prefer to put some abstract method in a common ancestor class so that I can later rely on how things quack rather than on what the interpreter believes them to be.

In Java each class resolves to its fully qualified class name. These are unique within a program and tests for instances are a breeze. In Python these can be slippery.

4
  • 2
    The problem you are describing occurs when the same module is imported twice under different names and Python cannot tell that those two names reference the same file. This is a bug in your code, not a problem with isinstance. This is also exactly why you don't put the internals of your package into PYTHONPATH (or sys.path). Dec 17, 2020 at 23:56
  • Geez, I've already written it's all about imports. The "you have a bug" point becomes less valid if you work with a library or someone else's code and there is no time to drill down to the bottom of that sort of behaviour.
    – bawey
    Dec 18, 2020 at 7:12
  • You might as well say "you've learned the hard way not to use if" because some library messed up the __bool__ method on some of their objects. Bugs in people's code is not a good reason to categorically stop using a core language feature, which isinstance absolutely is. Dec 18, 2020 at 22:10
  • Well, in all honesty I agree with you: it's often better to check explicitly if something is empty, is a None or, ironically, an instance of LocalProxy that ain't None until you start working on it (looking at you Flask and Werkzeug) rather than relying on mere if(my_var). My bias might be unfair, my wording poor but isinstance can be a hell of a surprise to someone coming from a different language background. If that makes me a mediocre dev - so be it - certain limits I'll never surpass. Meanwhile I simply prefer tools that don't trip me up. Cheers.
    – bawey
    Dec 19, 2020 at 11:10
2

If you don't want to use it, you've got other options. Traditional duck typing solution:

try:
    clickedblock_place = clickedblock.place
except AttributeError:
    # don't place block
else:
    clickedblock_place()

Or you can use hasattr:

if hasattr(clickedblock, 'place'):
    clickedblock.place()

I hardly ever use isinstance except for checking up (or is it down?) the inheritance hierarchy, say, for instance, if you need to know if a name points to a str OR a unicode:

if isinstance(str1, basestring):
    blah, blah, blah
3
  • 4
    Please don't use except:! It's terrible, horrible, and it will probably give your children some incurable desease. Using except: will catch all exceptions, which will make debugging legitimate bugs in .place() a nightmare, catch ctrl-c, and a host of other undesirable things. Nov 29, 2012 at 23:36
  • 2
    If you want to use try/except (not necessarily bad), use: try: place_block = clickedblock.place; except AttributeError: dont_place(); else: place_block() - that way you will see legitimate bugs in the .place() method. Nov 29, 2012 at 23:38
  • 2
    Thanks for the edit @David Wolever. You are of course exactly right. I was in a hurry.
    – MikeHunter
    Nov 30, 2012 at 0:24
1

I think I'd change it to be more like:

PLACEABLE_TYPES = [ Dirt ]
if isinstance(clickedblock, PLACEABLE_TYPES):
   place the block
else:
   don't place the block

but the idea in the comments of:

if clickedblock.is_placeable(that_place):
    place the block
else:
    don't place the block

also has merit.

1
  • 1
    The first example is almost certainly incorrect! It will fail if the clickedblock is a subclass of Dirt. Better would be: if isinstance(clickedblock, PLACEABLE_TYPES). But the second example, clickedblock.is_placeable(…), is likely much better. Nov 29, 2012 at 23:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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