Implementation of the visitor pattern

Question

I tried implementing visitor patter and have questions about my usage of is and as keywords. Is there a big performance hit when using them?

 class Program
{
 static void Main(string[] args)
 {
 List<IVisitor> visitors = new List<IVisitor>();
 visitors.AddRange(new List<IVisitor>()
 {
 new PersonVisitor(),
 new AnimalVisitor()
 });
 List<Creature> creatures = new List<Creature>();
 creatures.AddRange(new List<Creature>()
 {
 new Person() { Name = "Frank" },
 new Person() { Name = "Tony" },
 new Person() { Name = "Amy" },
 new Animal() { Name = "Bubbles" },
 new Animal() { Name = "Max" }
 });
 CreatureProcessor creatureProcessor = new CreatureProcessor(creatures, visitors);
 creatureProcessor.Process();
 Console.ReadKey();
 }
}
interface IVisitor
{
 void Visit(IElement element);
}
interface IElement
{
 void Accept(IVisitor visitor);
}
class PersonVisitor : IVisitor
{
 public void Visit(IElement element)
 {
 if (element is Person)
 {
 Creature creature = element as Person;
 Console.WriteLine("{0} is a good {1}", creature.Name, "Person");
 }
 }
}
class AnimalVisitor : IVisitor
{
 public void Visit(IElement element)
 {
 if (element is Animal)
 {
 Creature creature = element as Animal;
 Console.WriteLine("{0} is a good {1}", creature.Name, "Animal");
 }
 }
}
class Person : Creature
{
}
class Animal : Creature
{
}
abstract class Creature : IElement
{
 public string Name { get; set; }
 public void Accept(IVisitor visitor)
 {
 visitor.Visit(this);
 }
}
class CreatureProcessor
{
 private readonly List<IVisitor> visitors;
 private readonly List<Creature> creatures; 
 public CreatureProcessor(List<Creature> creatures, List<IVisitor> visitors)
 {
 this.creatures = creatures;
 this.visitors = visitors;
 }
 public void Process()
 {
 foreach (IVisitor visitor in visitors)
 {
 foreach (Creature creature in creatures)
 {
 creature.Accept(visitor);
 }
 }
 }
}

It works, the way it should I think, so would anyone care to critique it?

• Jamal: The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask.
• rjnilsson: If all you're interested in is the performance issue of casting the title is misleading. There's a pretty exhaustive description of the pros and cons of is, as and prefix casting here.
• Adriano Repetti: @Liger86 side question: I think this code is too fictional. Yes it may be formally correct (few minor changes unrelated to your question) but...why would you use it here? That code would be better without visitor pattern (the way it's actually implemented)...
• Heslacher: Although I lost some points you made the right choice by accepting this very good answer!

Answer 1

The Visitor Pattern

The main purpose of the Visitor pattern is to define extra elements of functionality for a number of classes in a single place. Your PersonVisitor and AnimalVisitor don't really demonstrate this at all - the name indicates that each visitor corresponds to a target type, not a piece of functionality. Better would be a single PrintIsGoodVisitor (probably with a better name). And given that your Visitor will be visiting IElement instances, IVisitor should probably be renamed IElementVisitor too.

interface IElementVisitor
{
 void Visit(IElement element);
}
class PrintIsGoodVisitor : IElementVisitor
{
 public void Visit(IElement element)
 {
 if (element is Person)
 {
 Creature creature = element as Person; 
 Console.WriteLine("{0} is a good {1}", creature.Name, "Person");
 }
 if (element is Animal)
 {
 Creature creature = element as Animal; 
 Console.WriteLine("{0} is a good {1}", creature.Name, "Animal");
 }
 }
}

You've now got a nasty code smell - passing in an IElement and checking: "Is it a Person? Do this", "Is it an Animal? Do that". The conventional way of implementing a Visitor is to call a virtual method on the visitee to determine its runtime type before the visitee tells the visitor to come and visit (that could have been less confusing...). This involves making Creature.Accept abstract and overriding it in the concrete subclasses.

abstract class Creature : IElement
{
 public string Name { get; set; }
 public abstract void Accept(IElementVisitor visitor);
}
class Person : Creature
{
 public override void Accept(IElementVisitor visitor)
 {
 visitor.Visit(this);
 }
}
class Animal : Creature
{
 public override void Accept(IElementVisitor visitor)
 {
 visitor.Visit(this);
 }
}

This may look like a bit of a waste of time -- every Creature was calling visitor.Visit(this) before, and they're all doing the same now -- but there is a difference. If we change IElementVisitor to have a Visit method for every concrete type that derives from IElement, then each of those concrete types knows which of those Visit methods it can call based on its own type:

interface IElementVisitor
{
 void Visit(Person person);
 void Visit(Animal animal);
}
class PrintIsGoodVisitor : IElementVisitor
{
 public void Visit(Person person)
 { 
 Console.WriteLine("{0} is a good {1}", person.Name, "Person");
 }
 public void Visit(Animal animal)
 {
 Console.WriteLine("{0} is a good {1}", animal.Name, "Animal");
 }
}

So when a Person says visitor.Visit(this), it can only be the first version, because this is a Person, and when an Animal says it, it can only be the second.

With these changes, we arrive at a program which is functionally identical to your original, but without a cast in sight - dynamic dispatch has taken their place.

With the Visitor side of things looked at, some points on the rest of the code:

Initializing Lists

In Main, you're creating a list specifically so you can add its elements to a second list. You can just use the second list directly. Additionally, when using a parameterless constructor and an initializer list, you can omit the constructor brackets:

List<Creature> creatures = new List<Creature>
{
 new Person { Name = "Frank" },
 new Person { Name = "Tony" },
 new Person { Name = "Amy" },
 new Animal { Name = "Bubbles" },
 new Animal { Name = "Max" }
};

var

It's considered good practice by many to use var where you can (at the very least for non built-in types) - it's a form of DRY to not repeat the type name twice:

var creatures = new List<Creature>
{
 new Person { Name = "Frank" },
 new Person { Name = "Tony" },
 new Person { Name = "Amy" },
 new Animal { Name = "Bubbles" },
 new Animal { Name = "Max" }
};

Principle of Least Privilege

This principle says that you should use the most general argument types you can for your methods. What is your CreatureProcessor going to do with its visitors and creatures lists? Enumerate their contents. So maybe IEnumerable<T> is a good fit here?

However, it's bad practice to iterate over an IEnumerable<T> more than once (it could be generating data from database calls for all we know!), and not only is Process potentially iterating over creatures multiple times, but there's nothing to stop Process from being called repeatly either. I'd suggest the next most general type that implies the data is safe to iterate repeatedly - IReadOnlyCollection<T>.

Using a more general type means that it can be used with collections other than List<T>, and signals to the user that you're not going to modify the collection you've been given (which List<T> can imply). (See this question for more discussion).

class CreatureProcessor
{
 private readonly IReadOnlyCollection<IVisitor> visitors;
 private readonly IReadOnlyCollection<Creature> creatures; 
 public CreatureProcessor(IReadOnlyCollection<Creature> creatures, IReadOnlyCollection<IVisitor> visitors)
 {
 this.creatures = creatures;
 this.visitors = visitors;
 }
 ...
}

Unnecessary Complexity

The CreatureProcessor type seems redundant - you pass two collections into its constructor which it stores, then you call a single method on it. Until you need something more complicated (see YAGNI), a single method alongside Main would suffice here. Even inlining it in Main is a possibility, but it feels sufficiently like a single responsibility that it deserves its own method.

class Program
{
 ...
 private static void VisitAllCreatures(IReadOnlyCollection<IElementVisitor> visitors, IReadOnlyCollection<Creature> creatures)
 {
 foreach (var visitor in visitors)
 {
 foreach (var creature in creatures)
 {
 creature.Accept(visitor);
 }
 }
 }
}

Also, on a similar theme, IElement isn't really doing a lot for you. It's directly implemented once and never used anywhere that Creature wouldn't be valid. It can be omitted (renaming IElementVisitor to ICreatureVisitor too).

Little Things

• I believe it's a good idea to be explicit about access modifiers (Stack Overflow question). I've gone with whatever the default value is, leaving the visibility unchanged from your original.
• Creature.Name is only ever set at construction and read thereafter, so could be changed to a readonly property set through a constructor - less mutability is less scope for bugs. You can add named parameters during construction if there could otherwise be confusion over what "Frank" represents. The new Creature constructor should be protected because the class is abstract and can't be instantiated.
• I'm also a believer of Josh Bloch's "Design and document for inheritance or else prohibit it". If you don't intend to have a class inherited from, mark it sealed so if someone does decide they want to do that, they know they may need to tweak the base class to make it work correctly.

The Final Code

internal abstract class Creature
{
 protected Creature(string name)
 {
 Name = name;
 }
 public string Name { get; }
 public abstract void Accept(ICreatureVisitor visitor);
}
internal sealed class Person : Creature
{
 public Person(string name)
 : base(name)
 {
 }
 public override void Accept(ICreatureVisitor visitor)
 {
 visitor.Visit(this);
 }
}
internal sealed class Animal : Creature
{
 public Animal(string name)
 : base(name)
 {
 }
 public override void Accept(ICreatureVisitor visitor)
 {
 visitor.Visit(this);
 }
}
internal interface ICreatureVisitor
{
 void Visit(Person person);
 void Visit(Animal animal);
}
internal sealed class PrintIsGoodVisitor : ICreatureVisitor
{
 public void Visit(Person person)
 { 
 Console.WriteLine("{0} is a good {1}", person.Name, "Person");
 }
 public void Visit(Animal animal)
 {
 Console.WriteLine("{0} is a good {1}", animal.Name, "Animal");
 }
}
internal sealed class Program
{
 public static void Main(string[] args)
 {
 var visitors = new List<ICreatureVisitor>
 {
 new PrintIsGoodVisitor()
 };
 var creatures = new List<Creature>
 {
 new Person(name: "Frank"),
 new Person(name: "Tony"),
 new Person(name: "Amy"),
 new Animal(name: "Bubbles"),
 new Animal(name: "Max")
 };
 VisitAllCreatures(visitors, creatures);
 Console.ReadKey();
 }
 private static void VisitAllCreatures(IReadOnlyCollection<ICreatureVisitor> visitors, IReadOnlyCollection<Creature> creatures)
 {
 foreach (var visitor in visitors)
 {
 foreach (var creature in creatures)
 {
 creature.Accept(visitor);
 }
 }
 }
}

Answer 2

Only focusing on is and as

class PersonVisitor : IVisitor
{
 public void Visit(IElement element)
 {
 if (element is Person)
 {
 Creature creature = element as Person;
 Console.WriteLine("{0} is a good {1}", creature.Name, "Person");
 }
 }
}

The is is basically doing the same as the as. It is doing a softcast, a cast which isn't throwing an exception, on the object in question. The difference is that the is is returning a bool stating the result and the as is returning for success the casted object and otherwise null. So you are doing the same thing twice.

A better and faster way would be like so

class PersonVisitor : IVisitor
{
 public void Visit(IElement element)
 {
 Creature creature = element as Person;
 if (creature != null)
 {
 Console.WriteLine("{0} is a good {1}", creature.Name, "Person");
 }
 }
}