Do I use Nested Classes to prevent class instantiation or...?

I have read several articles on when to use nested classes, but none that I've found address my specific question.

C# has a class called XmlReader which only provides a Create() method. I'm assuming that the create creates a subclass of XmlReader. If not, then for this example, assume that it does.

Consider this relationship:

/// <summary>
/// Class to read information in a file on disk
/// </summary>
interface ILoad
{
  /// <summary> Version number of the file </summary>
  int Version {get;}

  /// <summary> Content of the file </summary>
  string Content {get;}

  /// <summary> Full path to the file </summary>
  string FullPath {get;}
}

/// <summary> Provides base loading functionality </summary>
class LoaderBase : ILoad
{
    public int Version {get; protected set;}
    public string Content {get; protected set;}
    public string FullPath{get; protected set;}

    /* Helpers omitted */
    protected abstract void Load(string pathToFile);

    public static LoaderBase Create(string pathToFile)
    {
      switch(Path.GetExtension(pathToFile))
      {
         // Select the correct loader based on the file extension and return
      }
      return null;//unknown file type
    }
}

/// <summary> Base class functionality to load compiled files </summary>
public abstract class CompiledLoaderBase : LoaderBase
{
  protected CompiledLoaderBase(string path)
  {
    Load(path);
  }

  protected override Load(string path)
  {
     /* read the file and create an XmlReader from it */
     ReadVersionNumber(reader);
     ReadContent(reader); 
  }

  protected abstract void ReadVersionNumber(XmlReader reader);
  protected abstract void ReadContent(XmlReader reader);

  // Wish I could call this Create, but inherited a static Create method already
  public static CompiledLoaderBase CreateCompiled(string path)
  {
     //Figure out which loader to create and return it
     // ... Assume we figured out we need V1
     return new CompiledLoaderV1(path);
  }


  // Here's the fun stuff!
  protected class CompiledLoaderV1 : CompiledLoaderBase
  {
    public CompiledLoaderV1(string path)
    : base(path)
    {}

    protected override ReadVersionNumber(XmlReader reader)
    { /* read the version number and store in Version */ }

    protected override ReadContent(XmlReader reader)
    { /* read the content and store in Content */ }
  }

  // ... More classes with their own methods for reading version and content

}

Now, I used nested classes to prevent the user from creating the specific loaders directly; they must use one of the abstract base's Create* methods. FxCop blew up in my face over this and I was hoping to get some clarification on why.

It mentioned not to use nested classes, but instead namespaces. Is there a way to accomplish this with namespaces?

EDIT: Specifically, the message is: "NestedTypesShouldNotBeVisible". Resolution: "Do not nest type 'CompiledLoaderBase+CompiledLoaderV1'. Alternatively, change its accessibility so that it is not externally visible." Info: "Do not use public, protected, or protected internal nested types as a way of grouping types. Use namespaces for this purpose. There are very limited scenarios where nested types are the best design." Now, I believe Jon Skeet identified that you cannot accomplish this w开发者_高级运维ith namespaces. I just wanted to make sure since this error says that there are a limited scenarios where this is the best design, so if there is a better one, I'm open to ideas :D

Also, it did not like virtual call chain called from constructor. Is there a reason for this? Is there a way around it? EDIT: Specifically, the message is: "DoNotCallOverridableMethodsInConstructors". Resolution: "'CompiledLoaderV2.CompiledLoaderV2(String)' contains a call chain that results in a call to a virtual method defined by the class. Review the following call stack for unintended consequences" Info: "Virtual methods defined on the class should not be called from constructors. If a derived class has overridden the method, the derived class version will be called (before the derived class constructor is called)". I feel like this could be a problem if the subclasses did something in their constructors, but since they do not, I'm not sure this is an issue. Is there a better way to force the classes to load in a certain way without using abstract methods in the constructor?

Thanks so much for all your help!

No, you can't do this with namespaces, although you can do it with assemblies - i.e. prevent anyone outside the assembly from creating an instance.

You can absolutely do it with nested classes, but you generally should make the constructor itself private to prevent anything else deriving from the class. You can also make the nested classes themselves private unless you need to them to the outside world.

You can use this pattern to create something like Java enums, and also limited factories. I've used it for a discriminated union in Noda Time - the actual details don't matter, but you might like to look at the source for more inspiration.

You're right to mistrust calling virtual methods from constructors. It can occasionally be useful but should be done very carefully with heavy documentation.

Consider making the classes internal. In this way they can then be instantiated within your assembly, but not by clients of your library. For testing purposes you can make a test assembly an explicit friend of your assembly so it can "see" internal types, and create instances of them - much better for testing.

Here's a rough idea. What if the constructor is public (allowing you to call it), but requires something the user can't get.

public interface ILoad
{
}

public abstract class LoaderBase : ILoad
{
    public LoaderBase(InstanceChooser dongle)
    {
      if (dongle == null)
      {
        throw new Exception("Do not create a Loader without an InstanceChooser");
      }
    }
    public abstract void Load(string path);
}

public class InstanceChooser
{
    private InstanceChooser()
    {
    }

    //construction and initialization
    public static ILoad Create(string path)
    {
        InstanceChooser myChooser = new InstanceChooser();
        LoaderBase myLoader = myChooser.Choose(path);
        if (myLoader != null)
        {
            myLoader.Load(path); //virtual method call moved out of constructor.
        }
        return myLoader;
    }

    //construction
    private LoaderBase Choose(string path)
    {
        switch (System.IO.Path.GetExtension(path))
        {
            case "z":  //example constructor call
                return new CompiledLoaderV1(this);
        }
        return null;
    }
}

public class CompiledLoaderV1 : LoaderBase
{
    public CompiledLoaderV1(InstanceChooser dongle)
        : base(dongle)
    {
    }

    public override void Load(string path)
    {
        throw new NotImplementedException();
    }
}

PS, I hate returning null. Feels so much better to throw and not have to write a million null checks.

Edited: Here is an example:

class Program
{
    static void Main(string[] args)
    {
        Shape shape = Shape.Create(args[0]);
    }
}

public abstract class Shape
{
    protected Shape(string filename) { ...  }
    public abstract float Volume { get; }
    public static Shape Create(string filename)
    {
        string ext = Path.GetExtension(filename);
        // read file here
        switch (ext)
        {
            case ".box":
                return new BoxShape(filename);
            case ".sphere":
                return new SphereShape(filename);
        }
        return null;
    }
    class BoxShape : Shape
    {
        public BoxShape(string filename)
            : base(filename)
        {
            // Parse contents 
        }
        public override float Volume { get { return ... } }
    }
    class SphereShape : Shape
    {
        float radius;
        public SphereShape(string filename)
            : base(filename)
        {
            // Parse contents
        }
        public override float Volume { get { return ... } }
    }
}

it creates instances of Shape using nested classes for concrete classes, so that the user never bothers with the derived classes. The abstract class chooses the correct implementation and arguments based on the file extension and file contents.