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);
}
}
}
}
