Is isinstance a good way of identifying the type of an object?

Question

I have a bunch of classes inheriting from Violation. These subclasses model violations to different rules: UsedTimeslot, TeamConstraint, etc...

I need to check what kind of violation happened in order to respond to it:

if team_constraint_violation_ocurred:
    corresponding_action()

However, the only mechanism I have of checking what type of violation occurred is checking the class type:

if isinstance(violation, TeamConstraintViolation):
        corresponding_action()

Is this a proper way of identifying the type of an object? Or should I introduce a type attribute, like an enum? Or should I use a different design altogether?

Answer 1

If you really need to check the type of objects, sure, use isinstance - that's what it was invented for.

However, if your real requirement is do something different depending on the subtype of an object, then usually it's a better idea to implement this code as a method in each subclass and simply call that method on the object of dynamic type. That way, dynamic dispatch takes care of the conditionals and you don't have to write them by hand. For instance, you can define an abstract method respond() in Violation and then have each specific type of Violation override it to perform a type-specific response.

It can happen that the polymorphic class is manifestly the wrong place to put that code. In that case, you can compromise and write a dynamically dispatched method that does only part of the work. For instance, it might generate a detailed description of the error and hand it to the caller for dispatch via your logging mechanism. Or it might decide which course of action is appropriate (e.g. SyntaxError: fail, LexicalError: fail, TimeoutError: retry) and signal the caller what to do. In the extreme case the Violation could at least have a dynamic getType() method so that you can switch over its return value, rather than have to write out isinstance N times.

Answer 2

What you need is not to know what type of violation is thrown, but rather what action to invoke. There are various ways to do this. @KilianFoth suggested a very reasonable way to do this. However, if you do not want to make ExecuteAction a method of Violation class, you could use a Visitor pattern, which would look like this:

    public interface IVisitor
{
    void VisitViolationA(ViolationA a);
    void VisitViolationB(ViolationB b);
    void VisitViolationC(ViolationC c);

}

public abstract class Violation
{
    public abstract void Accept(IVisitor visitor);
}

public class ViolationA : Violation
{
    public override void Accept(IVisitor visitor)
    {
        visitor.VisitViolationA(this);
    }

}

public class ViolationB : Violation
{
    public override void Accept(IVisitor visitor)
    {
        visitor.VisitViolationB(this);
    }

}

public class ViolationC : Violation
{
    public override void Accept(IVisitor visitor)
    {
        visitor.VisitViolationC(this);
    }

}

public class ActionForViolation : IVisitor
{
    public void VisitViolationA(ViolationA a)
    {
        //Corresponding action for ViolationA
    }

    public void VisitViolationB(ViolationB b)
    {
        //Corresponding action for ViolationB
    }

    public void VisitViolationC(ViolationC c)
    {
        //Corresponding action for ViolationC
    }
}


public class SomeOtherActionForViolation : IVisitor
{
    public void VisitViolationA(ViolationA a)
    {
        //Some other corresponding action for ViolationA
    }

    public void VisitViolationB(ViolationB b)
    {
        //Some other corresponding action for ViolationB
    }

    public void VisitViolationC(ViolationC c)
    {
        //Some other corresponding action for ViolationC
    }
}


public class Client
{
    public void MethodWithViolations()
    {
        ViolationA violationA = new ViolationA();
        ActionForViolation afv = new ActionForViolation();
        violationA.Accept(afv);
    }

    public void SomeOtherMethodWithViolations()
    {
        ViolationA violationA = new ViolationA();
        SomeOtherActionForViolation soafv = new SomeOtherActionForViolation();
        violationA.Accept(soafv);
    }
}

With this approach, you have decoupled actions taken when violations occur from violations themselves, and you have provided yourself with the possibility to define different actions for same violations, should the need occur. The Violation class derivates are only responsible to invoke the appropriate IVisitor method. The obvious downside to this is that IVisitor interface can be very big in case there are many Violations, but in that case, you really should reconsider putting the violation handling in the Violation class and its derivates.

Answer 3

You can also use type(violation).__name__ and if you know what you're looking for, you can define various actions:

# define actions and mapping
def actionOne():
    #some action

def actionTwo():
    #some action

def actionThree():
    #some action

violations = {
    TeamConstraintViolation: actionOne,
    UnauthorizedViolation: actionTwo,
    MaxQuotaViolation: actionTwo,
    NaughtyLanguageViolation: actionThree
}

# check violation and take action
violations[type(violation).__name__]()

The advantage of this approach is you might have a finite number of actions to take and many violations may result in the same action.