Implementation of the visitor pattern

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 visitors = new List();
visitors.AddRange(new List()
{
new PersonVisitor(),
new AnimalVisitor()
});

List creatures = new List();
creatures.AddRange(new List()
{
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 visitors;
private readonly List creatures;

public CreatureProcessor(List creatures, List visitors)
{
this.creatures = creatures;
this.visitors = visitors;
}

public void Process(

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 creatures = new List
{
    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
{
    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 is a good fit here?

However, it's bad practice to iterate over an IEnumerable 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.

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

```
class CreatureProcessor
{
private readonly IReadOnlyCollection visitors;

StackExchange Code Review Q#108001, answer score: 6