c# Public Nested Classes or Better Option?

Question

I have a control circuit which has multiple settings and may have any number of sensors attached to it (each with it's own set of settings). These sensors may only be used with the control circuit. I thought of using nested classes like so:

public class ControlCircuitLib
{
    // Fields.
    private Settings controllerSettings;
    private List<Sensor> attachedSensors;

    // Properties.
    public Settings ControllerSettings
    { get { return this.controllerSettings; } }

    public List<Sensor> AttachedSensors
    { get { return this.attachedSensors; } }

    // Constructors, methods, etc.
    ...

    // Nested classes.
    public class Settings
    {
       // Fields.
       private ControlCircuitLib controllerCircuit;
       private SerialPort controllerSerialPort;
       private int activeOutputs;
       ... (many, many more settings)

       // Properties.
       public int ActiveOutputs
       { get { return this.activeOutputs; } }
       ... (the other Get properties for the settings)

       // Methods.
       ... (method to set the circuit properties though serial port)        
    }

    public class Sensor
    {
       // Enumerations.
       public enum MeasurementTypes { Displacement, Velocity, Acceleration };

       // Fields.
       private ControlCircuitLib controllerCircuit;
       private string sensorName;
       private MeasurementTypes measurementType;
       private double requiredInputVoltage;
       ... (many, many more settings)

       // Properties.
       public string SensorName {...}
       ... (Get properties)

       // Methods.
       ... (methods to set the sensor settings while attached to the control circuit)
    }
}

I have read that public nested classes are a "no-no" but that there are exceptions. Is this structure OK or is there a better option?

Thanks!

EDIT

Below is a crude hierarchy of the control circuit for which I am trying to write a library class for; I used code formatting to prevent text-wrap.

Control Circuit (com. via serial port) -> Attached Sensors (up to 10) -> Sensor Settings (approx. 10 settings per sensor)
                                          Basic Controller Settings (approx. 20 settings)
                                          Output Settings (approx. 30 settings)
                                          Common Settings (approx. 30 settings)
                                          Environment Settings (approx. 10 settings)

All of the settings are set through the controller but I would like an organized library instead of just cramming all ~100 methods, properties, and settings under one Controller class. It would be HUGELY appreciated if someone could offer a short example outlining the structure they would use. Thanks!

Anodize

How do you "configure" the control circuit? Do you create sensors internally based on the settings? Are the settings what define a configuration of sensors? – Monday

@Jordão: Basically, the control circuit has built in serial functions which are used to set the circuit's (and the attached sensor's) settings. I want to make a library class so that I don't have to manually create the command strings (e.g. SetSensorSettingA(sensorInstance, -20) instead of sending "NA,BL,01,-20,\r"). I am a Mech. Eng. so I just wanted to know how real coders would organize this class :) – Anodize

any final solution with full source code sample working? – Bacteriology

Answer 1

The contents of a class should be the implementation details of that class. Are the nested classes implementation details of the outer class, or are you merely using the outer class as a convenient name scoping and discovery mechanism?

If the former, then you shouldn't be making the private implementation details publically available. Make them private if they are implementation details of the class.

If the latter, then you should be using namespaces, not outer classes, as your scoping and discovery mechanism.

Either way, public nested classes are a bad code smell. I'd want to have a very good reason to expose a nested class.

Danyelldanyelle

The public methods of a class are also part of the API of a class though - they shouldn't just be regarded as implementation details surely, particularly if they're not specified in (say) an interface. Is it so awful for types only used within the API of the outer class to be declared as nested types? It's not something I'd regularly do, but I think in the pre-2.0 days it would have made sense for a delegate type only intended to be used for events declared in that type, for example. – Girlhood

(In case anyone's in any doubt, I suspect I'll end up agreeing with Eric in the end, as usual - but I just wanted to explore the ideas in a bit more detail.) – Girlhood

Hmmm., I don't really follow. Here was my logic. These sensors can ONLY be used with a specific circuit and you can't get/set any sensor settings without going through the controller circuit; I want to show that relationship. Also, I don't even want to expose a Sensor constructor (the controller will have a method for this) but I want to be able to "save" a Sensor instance. E.g., 'ControlCircuitLib.Sensor sensorOne = controllerCircuitInstance.AttachedSensors[0];`. Would you mind showing a very short example of how you would structure this class? – Anodize

@JonSkeet: Well it is not awful, but it is certainly unusual for a nested type to be part of the public "API" surface of a class. – Danyelldanyelle

@EricLippert: Right - I'd go along with that. Delegates for class-specific events are the example I came up with as the most likely use case, and the generic events in .NET 2 have removed that as a problem for the most case. Admittedly in Noda Time I've got various internal nested classes, but that's a different matter... – Girlhood

@Jon: It's not clear to me that the type being nested indicates the command-and-control hierarchy. Take Reflection for example. Suppose there is no way to get a ParameterInfo except via a MethodInfo, no way to get a MethodInfo except via a Type and no way to get a Type except via an Assembly. Do you want to have four levels of type nesting, where Type is a nested type of Assembly, MethodInfo is a nested type of Type, and so on? – Danyelldanyelle

It does have some interesting impact on generic outer classes, you cannot create/invoke to the inner class in any meaningful way without supplying a concrete specification for the generic types. Whether this is important is subjective of course. This is likely not that important in terms of exposing things. I actually think its a good way of "obscuring" classes which are indeed implementation details but annoyingly must be public. Think struct based enumerators in collections. You really shouldn't ever have to ever have them pop up in your intellisense just because you imported S.C.G – Blameful

I've seen it stated several times that we "should be using namespaces, not outer classes, as your scoping and discovery mechanism", but no one's given me a reason for this. – Botfly

@DCShannon: The purpose of the namespace feature is to organize types, and the purpose of the nested type feature is to enable use of types as private implementation details of other types. So the reason for that advice is a consequence of the more general good advice: don't use features to work against their intended purposes. – Danyelldanyelle

What I just heard is "because Microsoft says so", and frankly that's not very compelling compared to an actual reason related to how useful the practice either is or isn't. – Botfly

@DCShannon: Do you disagree with the advice "don't use features to work against their intended purposes"? This advice applies well to a great many domains, from medicine through carpentry. Taking drugs "off label" to treat conditions they were not designed to treat, or using a hammer to drive screws, are both poor practices. One could give a detailed justification for why it's a bad idea to drive screws with a hammer, but at this point I'm not sure that any explanation I'd give would be satisfying. – Danyelldanyelle

@DCShannon: Moreover, what entity, if not Microsoft, would you expect to set guidelines for proper, expected and effective use of tools created by Microsoft for Microsoft's customers? "Because Microsoft says so" seems like a great reason to follow a guideline for use of a Microsoft product. – Danyelldanyelle

All warranties contained herein are null and void if the equipment was not used in accordance with the manufacturer’s instructions... :) – Lawsuit

Answer 2

I don't have too much problem with public nested classes (I'm not a fan of dogmatic rules, in general) but have you considered putting all of these types in their own namespace instead? That's the more common way of grouping classes together.

EDIT: Just to clarify, I would very rarely use public nested classes, and I probably wouldn't use them here, but I wouldn't completely balk at them either. There are plenty of examples of public nested types in the framework (e.g. List<T>.Enumerator) - no doubt in each case the designers considered the "smell" of using a nested class, and considered it to be less of a smell than promoting the type to be a top-level one, or creating a new namespace for the types involved.

Girlhood

A namespace around all of the classes is a good idea, but you cannot just make the outer class a namespace as it contains data. – Only

@drdwilcox: No, I wasn't suggesting that. I meant all the types which are shown, i.e. including ControlCircuitLib (which should probably be renamed). – Girlhood

I recognized that. I was just adding some specificity, mostly because of the name of the outer class. – Only

Answer 3

From your comment to Eric's answer:

These sensors can ONLY be used with a specific circuit

This kind of relationship is commonly known as a dependency. The Sensor constructor should take a ControlCircuit as a parameter. Nested classes do not convey this relationship.

and you can't get/set any sensor settings without going through the controller circuit;

I think that means that all Sensor properties will delegate to (call) or somehow inform (fire an event on) the ControlCircuit when they're used. Or, you'd have some kind of internal interface to the sensor that only the control circuit uses, making Sensor an opaque class to the outside world. If that's the case, Sensor is just an implementation detail and could be nested private or internal (there's also no need to "save" a sensor instance if you can't do anything with it).

Also, I don't even want to expose a Sensor constructor (the controller will have a method for this)

The fact that the Sensor constructor now takes a control circuit is enough of a hint as to what depends on what that you could leave the constructor public. You can also make it internal.

A general comment that I have is that this design is very coupled. Maybe if you had some interfaces between control circuit, sensor and settings, it would be easier to understand each component independently, and the design would be more testable. I always find beneficial to make the roles that each component plays explicit. That is, if they're not just implementation details.

Panayiotis

Yes, my Sensor class actually does take a ControlCircuit as a parameter. Also, I communicate with the control circuit via serial port and use it to get/set the various controller (and the attached sensor) settings; I need to have a publicly available class to "store" an attached sensors settings. For example: Sensor sensor1Settings = controllerInstance.AttachedSensors[0];. I have edited my original post to include the circuit's hierarchy, maybe that will help illustrate what I am trying to accomplish. I am a mech. eng. so I am trying to write this class like a real programmer would :) – Anodize

Then it looks like your Sensor should really be public. But you don't need it to be nested to convey that it depends on the ControlCircuit. – Monday

Answer 4

I would say the better option is moving those nested classes out of the class they're in and have them stand on their own. Unless I'm missing something you appear only to have them in the main class in order for some sort of scoping concept, but really, that's what namespaces are for.

Betatron

Answer 5

I generally disagree with Eric on this.

The thing I usually consider is: how often should the end user use the type name ControlCircuitLib.Sensor. If it's "almost never, but the type needs to be public so that doing something is possible", then go for inner types. For anything else, use a separate type.

For example,

public class Frobber {
    public readonly FrobType Standard = ...;
    public readonly FrobType Advanced = ...;

    public void Frob(FrobType type) { ... }

    public class FrobType { ... }
}

In this example, the FrobType only acts as an opaque 'thing'. Only Frobber needs to know what it actually is, although it needs to be possible to pass it around outside that class. However, this sort of example is quite rare; more often than not, you should prefer to avoid nested public classes.

One of the most important things when designing a library is to keep it simple. So use whichever way makes the library and the using code simpler.

Anthropometry

Your example might be clearer if FrobType looked like something where applications would be more interested the fact that a FrobType has certain members than in knowing what it is. For example, if one has a method that returns a few values, returning a struct containing such values may be more efficient than having multiple ref parameters. Code could say var result=myThing.ComputeStuff(); if (result.Minvalue) ... if (result.MaxValue)... etc. If Result is a struct, adding members would require recompiling code, but if the only code which uses the structure type is... – Silverweed

...code which calls myThing.ComputeStuff() that might not be too bad. The situation would be much worse if other unrelated code also used the structure. Having the result type be a nested type within the type of myThing would help discourage such use; having it public (like e.g. KeyValuePair<TKey,TValue>, which mainly exists for Dictionary<TKey,TValue>, encourages it. – Silverweed

Answer 6

I like nested classes in cases like this because it shows the relationship. If you do not want users of the outer class to be able to create items of the inner class separately from the outer class, you can always hide the constructors and use factory methods in the outer class to create elements of the inner class. I use this structure a lot.

Only

Answer 7

This structure seems completely reasonable to me. I wasn't aware until today that Microsoft has advised against doing this, but I'm still not aware why they've advised as such.

I've used this structure in a situation where the nested class only exists to support the containing class (i.e. it's part of its implementation), but other classes need to be able to see it in order to interact with the containing class (i.e. it's part of the class's API).

That being said, Eric generally knows what he's talking about, so out of respect for his knowledge and for the time being, I've converted those classes to use namespaces instead.

Currently, I'm not liking the results. I have a class named BasicColumn, which exists only to represent a column in a class called Grid. Previously, that class was always addressed as Grid.BasicColumn, which was great. That's exactly how I want it to be referred to. Now, with the Grid and the BasicColumn both in the Grids namespace, it's just referred to as BasicColumn with a 'using Grids' at the top of the file. There's nothing to indicate its special relationship with Grid, unless I want to type out the entire namespace (which has a few prefixes before Grid I've left out for simplicity).

If anyone can point out an actual reason why using public nested classes is somehow counterproductive or suboptimal, other than the irrelevant fact that Microsoft doesn't intend for them to be used that way, then I'd love to hear it.

Botfly

I will add that I ended up using public nested classes for the exact same reason as you and I would argue it isn't actually a violation of what Eric is describing. The point of your nested classes is that they are scoped implementation details of the class they are nested in, they are only public to expose needed APIs. Ideally, you would just make the nested class protected or private and then have it implement a public interface which provides the API in. – Its

@AJHenderson That sounds like a plan. – Botfly

Answer 8

While I feel Eric's answer is correct, it is important to realize it doesn't really fully address what your situation is.

Your case sounds very similar to one I frequently find myself in where you have a class which is really implementation details of another class, however, some details or functionality of that sub-component naturally lend themselves towards being exposed directly for some minor aspects that are not governed by the parent.

In these cases, what you can do is use interfaces. The nested classes need not be public as they really are internal details of the class they are nested within, but a subset of functionality (an interface) needs to be made publicly available and can be implemented by the class.

This allows for construction of the internal structures to be controlled by the class they are nested within while still allowing direct access to the type from a namespace for external references. (The caller will use SomeNamespace.IChildApi as the name rather than SomeNamespace.NestingClass.NestedClass.SecondNestedClass.ThirdNestedClass, etc.)

Its