Question

What are the best practices regarding the use and structure of inner classes in C#.

For instance if I have a very large base class and two large inner classes should I split them up into separate (partial class) codefiles or leave them as one very large unwieldy codefile?

Also is it bad practice to have an abstract class, with a public inherited inner class?

Was it helpful?

Solution

Typically I reserve inner-classes for one of two purposes:

  1. Public classes which derive from their parent class where the parent class is an abstract base implementation with one or more abstract methods and each subclass is an implementation which serves a specific implementation. after reading Framework Design and Guidelines I see that this is marked as "Avoid", however I use it in scenarios similar to enums--althogh that's probably giving a bad impression as well

  2. The inner classes are private and are units of business logic or otherwise tightly coupled to their parent class in a manner in which they are fundamentally broken when consumed or used by any other class.

For all other cases I try to keep them in the same namespace and the same accessibility level as their consumer/logical parent--often with names that are a little less friendly than the "main" class.

On big projects you'd be surprised how often you may find yourself initially building a strongly-coupled component just because it's first or primary purpose makes it seem logical--however unless you have a very good or technical reason to lock it down and hide it from sight then there is little harm in exposing the class so that other components can consume it.

Edit Keep in mind that even though we're talking about sub-classes they should be more-or-less well designed and loosely coupled components. Even if they are private and invisible to the outside world keeping a minimal "surface area" between classes will greatly ease the maintainability of your code for future expansion or alteration.

OTHER TIPS

I don't have the book to hand, but the Framework Design Guidelines suggests using public inner classes as long as clients don't have to refer to the class name. private inner classes are fine: nobody's going to notice these.

Bad: ListView.ListViewItemCollection collection = new ListView.ListViewItemCollection();

Good: listView.Items.Add(...);

Regarding your large class: it's generally worthwhile splitting something like this into smaller classes, each with one specific piece of functionality. It's difficult to break it up initially, but I predict it'll make your life easier later on...

Generally inner classes should be private and usable only by the class that contains them. If they inner classes are very large that would suggest they should be their own classes.

Usually when you've got a big inner class it's because the inner class is tightly coupled to it's containing class and needs access to its private methods.

I think this is rather subjective, but I would probably split them up in separate code files by making the "host" class partial.

By doing like this, you can get even more overview by editing the project file to make the files group just like designer classes in Windows Forms. I think I've seen a Visual Studio addin that does this automagically for you, but I don't remember where.

EDIT:
After some looking I found the Visual Studio add-in for doing this called VSCommands

Regarding only how to structure such a beast ...

You can use partial classes to split the main class and the nested classes. When you do so, you're advised to name files appropriately so it's obvious what is going on.

// main class in file Outer.cs
namespace Demo
{
  public partial class Outer
  {
     // Outer class
  }
}

// nested class in file Outer.Nested1.cs
namespace Demo
{
  public partial class Outer
  {
    private class Nested1
    {
      // Nested1 details
    }
  }
}

In much the same way, you often see (explicit) interfaces in their own file. e.g. Outer.ISomeInterface.cs rather than the editor default of #regioning them.

Your projects file structure then starts to look like

   /Project/Demo/ISomeInterface.cs
   /Project/Demo/Outer.cs
   /Project/Demo/Outer.Nested1.cs
   /Project/Demo/Outer.ISomeInterface.cs

Typically when we're doing this it's for a variation of the Builder pattern.

I personally like to have one class per file, and the inner classes as part of that file. I believe inner classes should typically (nearly always) be private, and are an implementation detail of the class. Having them in a separate file confuses things, IMO.

Using code regions to wrap around the inner classes and hide their details works well for me, in this case, and keeps the file from being difficult to work with. The code regions keep the inner class "hidden", and since it's a private implementation detail, that's fine with me.

I personally use inner classes to encapsulate some of concepts and operations used only internally within a class. This way I don't pollute that class' non-public api and keep the api clean and compact.

You can take advantage of partial classes to move the definition of these inner classes into a different file for better orgnanization. VS does not automatically group partial class files for you except for some of the templatized items such as ASP.NET, WinForm forms, etc. You will need to edit the project file and make some changes in there. You can look at one of the existing grouping in there to see how it is done. I believe there are some macros that allow you to group partial class files for you in the solution explorer.

In my opinion inner classes, if needed at all, should be kept small and used internally by that class only. If you use Relfector on the .NET framework you'll see them used a lot just for that purpose.

If your inner classes are getting too big I would definitely move them out into separate classes/codefiles somehow if only for maintainability. I have to support some existing code where someone thought it was a great idea to use inner classes within inner classes. It resulted in an inner class hierarchy running 4 - 5 levels deep. Needless to say the code is impenetrable and takes ages to understand what you are looking at.

Here see a practical example of a nested class that could give you an idea of their use (added some unit test)

namespace CoreLib.Helpers
{
    using System;
    using System.Security.Cryptography;

    public static class Rnd
    {
        private static readonly Random _random = new Random();

        public static Random Generator { get { return _random; } }

        static Rnd()
        {
        }

        public static class Crypto
        {
            private static readonly RandomNumberGenerator _highRandom = RandomNumberGenerator.Create();

            public static RandomNumberGenerator Generator { get { return _highRandom; } }

            static Crypto()
            {
            }

        }

        public static UInt32 Next(this RandomNumberGenerator value)
        {
            var bytes = new byte[4];
            value.GetBytes(bytes);

            return BitConverter.ToUInt32(bytes, 0);
        }
    }
}

[TestMethod]
public void Rnd_OnGenerator_UniqueRandomSequence()
{
    var rdn1 = Rnd.Generator;
    var rdn2 = Rnd.Generator;
    var list = new List<Int32>();
    var tasks = new Task[10];
    for (var i = 0; i < 10; i++)
    {
        tasks[i] = Task.Factory.StartNew((() =>
        {
            for (var k = 0; k < 1000; k++)
            {
                lock (list)
                {
                    list.Add(Rnd.Generator.Next(Int32.MinValue, Int32.MaxValue));
                }
            }
        }));
    }
    Task.WaitAll(tasks);
    var distinct = list.Distinct().ToList();
    Assert.AreSame(rdn1, rdn2);
    Assert.AreEqual(10000, list.Count);
    Assert.AreEqual(list.Count, distinct.Count);
}

[TestMethod]
public void Rnd_OnCryptoGenerator_UniqueRandomSequence()
{
    var rdn1 = Rnd.Crypto.Generator;
    var rdn2 = Rnd.Crypto.Generator;
    var list = new ConcurrentQueue<UInt32>();
    var tasks = new Task[10];
    for (var i = 0; i < 10; i++)
    {
        tasks[i] = Task.Factory.StartNew((() =>
        {
            for (var k = 0; k < 1000; k++)
            {
                    list.Enqueue(Rnd.Crypto.Generator.Next());
            }
        }));
    }
    Task.WaitAll(tasks);
    var distinct = list.Distinct().ToList();
    Assert.AreSame(rdn1, rdn2);
    Assert.AreEqual(10000, list.Count);
    Assert.AreEqual(list.Count, distinct.Count);
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top