Is usage of isinstance is justified when calling different methods by passed object

Question

I'm working on little tool that is able to read file's low level data i.e mappings, etc, and store the results to sqlite DB, using python's builtin sqlite API.

For parsed file data I have 3 classes:

class GenericFile: # general file class 
    # bunch of methods here
    ...
class SomeFileObject_A: # low level class for storing objects of kind SomeFileObject_A
    # bunch of methods here
    ...
class SomeFileObject_B: # low level cass for storing objects of kind SomeFileObject_A
    # bunch of methods here
    ...

sqlite interface is implemented as a separate class:

class Database:
    def insert(self, object_to_insert):
    ...
    def _insert_generic_file_object(self, object_to_insert):
    ...
    def _insert_file_object_a(self, object_to_insert):
    ...
    def _insert_file_object_b(self, object_to_insert):
    ...
    # bunch of sqlite related methods

When I need to insert some object to DB, I'm using db.insert(object).

Now I thought that it could be good idea to use isinstance in my insert method, as it takes care of any inserted object, without need to explicitly call suitable method for each object, which looks more elegant. But after reading more on isinstance, I begin to suspect, that my design is not so good.

Here is implementation of generic insert method:

class Database:
    def insert(self, object_to_insert):
        self._logger.info("inserting %s object", object_to_insert.__class__.__name__)
        if isinstance(object_to_insert, GenericFile):
            self._insert_generic_file_object(object_to_insert)
        elif isinstance(object_to_insert, SomeFileObject_A):
            self._insert_file_object_a(object_to_insert)
        elif isinstance(object_to_insert, SomeFileObject_B):
            self._insert_file_object_b(object_to_insert)
        else:
            self._logger.error("Insert Failed. Bad object type %s" % type(object_to_insert))
            raise Exception
        self._db_connection.commit()

So, should isinstace be avoided in my case, and if it should, what is better solution here?

Thanks

That's mainly a question of code-style, which is hard to evaluate if the whole code is not here (I do not ask for whole code). I think that, if `isinstance` works fine and looks nice for your case and for separating responsabilities, use it. — DainDwarf, Dec 29 '15 at 08:30

score 1 · Accepted Answer · answered Dec 29 '15 at 08:57

One of the base principles in OO is to replace explicit switches with polymorphic dispatch. In your case, the solution would be to use double dispatch so it's the FileObect 's responsability to know which Database method to call, ie:

class GenericFile: # general file class 
    # bunch of methods here
    ...
    def insert(self, db):
        return db.insert_generic_file_object(self)


class SomeFileObject_A: # low level class for storing objects of kind SomeFileObject_A
    # bunch of methods here
    ...
    def insert(self, db):
        return db.insert_file_object_a(self)


class SomeFileObject_B: # low level cass for storing objects of kind SomeFileObject_A
    # bunch of methods here
    ...
    def insert(self, db):
        return db.insert_file_object_b(self)


class Database:
    def insert(self, obj):
        return obj.insert(self)

Is usage of isinstance is justified when calling different methods by passed object

1 Answers1