1

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?

8

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.

  • yeah I know delegating the behavior to the Violation is the ideal thing to do from an OOP perspective, but it just doesn't feel like executing those actions is a responsibility that should fall on the Violation class, and also it just doesn't have access to many necessary pieces to carry on these actions (these pieces lie on other classes/modules) – dabadaba Jun 23 '17 at 11:55
  • 3
    Another approach is to use the Visitor pattern. Have your Violation classes implement a method, usually called accept, that just dispatches back to a method in the object in its argument... But each implementation calls a different method (eg processUsedTimeSlot, processTeamConstraint, and so on). That way, you at least separate out the handling so you don't have a massive method with lots of isinstance conditions. – Jules Jun 23 '17 at 12:24
  • @Jules hmm did I picture it right? would the implementation be similar to this? pastebin.com/wXdTKyuy – dabadaba Jun 23 '17 at 13:49
  • The visitor pattern for languages that lack algebraic data types and pattern matching is a great solution to stop a simple data class from bloating with many unrelated behaviors. "make it an instance method instead of type checking" can lead to some extremely awkward interfaces when taken too far. – Daenyth Jun 23 '17 at 15:22
  • I would avoid the Visitor pattern. A much simpler solution is to add a method to the Violation class that returns an Action instance. The Action class encapsulates the details of what to do and can be as complicated as it needs to be. That is, you can add all kinds of behaviors to the Action classes that the Visitor class need know nothing about. Visitor is ugly and requires just as much coupling as this approach but in an obfuscated way. – JimmyJames Jun 23 '17 at 16:09
1

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.

1

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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