I'd like some clarification on my understanding on this one. At work i came across some code that was to do with a particle system, it looked like this (omitting the gubbins); The Config class was used to store details about the system, i.e. start and end colours, min and max life time of particles. It had only member variables and a constructor that initialises them. In the ParticleSystem class there was a member variable of the type config. Its probably a simple principle and one that i should know, but i can't get my head around why Config was declared inside the ParticleSytem class. Is it for clarity? In that the config class can only be associated with the ParticleSystem? If thats so then why have a class for that anyway, why not have Config's members as members of the ParticleSystem? Perhaps i'm missing the point all together and should be asking, when is it appropriate to declare a class within a class. Your help is much appreciated.

Well i personally try never to declare a class within a class. I think it makes things less readable etc. I certainly wouldn't declare a class within a class with public encapsulation. If it needs to be public because it needs to be used from elsewhere then put it in its own file.

I personally don't think i have come accross a scenario where declaring a class or struct within another class was necesary.

In this scenario i guess you might want to change the config of a particle system on the fly. Maybe if you had a collection of configs that could be a applied at any given time, it would be simple to set the config in 1 call if you wrap the data in a struct.

The logic of a seperate structure storing configuration information is for simplification, flexibility, and good design. For example, many particle systems are controlled by a large array of variables, which can be extremely tedious to set by hand in a constructor. Placing all those values in a wrapper class like Config allows cleaner access as well as allowing you to accept the default configuration for all but those values you'd like to override.

The logic for placing it inside the ParticleSystem class like that is to introduce scope. It's a particle system configuration class, as opposed to a general configuration class or a fluid system configuration class. It indicates the class is strongly associated with the particle system.

Quote:Original post by jpetrie
The logic of a seperate structure storing configuration information is for simplification, flexibility, and good design.

Ok so you mean that the configuration of this particular particle system can be switched my changing the ParticleSystems Config member variable. Instead of changing each element separately. This would also explain why Config's member variables are all declared public, which i thought was BAD design! Shows what i know.

Quote:Original post by jpetrie
The logic for placing it inside the ParticleSystem class like that is to introduce scope.

See i was thinking that it was written in a way to discourage creating Config objects outside of the ParticleSystem class. But what its actually saying is, you can creat them but i'm making you go through ParticleSystem::Config to make sure you understand its a config class for the particle system.

Quote:Original post by JimmyDeemo

Quote:Original post by jpetrie
The logic of a seperate structure storing configuration information is for simplification, flexibility, and good design.

Ok so you mean that the configuration of this particular particle system can be switched my changing the ParticleSystems Config member variable. Instead of changing each element separately. This would also explain why Config's member variables are all declared public, which i thought was BAD design! Shows what i know.

I believe this is bad design. Config should have accessors to its members so that it can do any necessary validation when setting their values. Either that or ParticlSystem's SetConfig() accessor validates it when it is set.

Quote:Original post by DaveConfig should have accessors to its members so that it can do any necessary validation when setting their values.

This is what i would have thought. I can only assume that on this occasion they were written as public for speed and simplicity over validation and encapsulation.

Quote:
Ok so you mean that the configuration of this particular particle system can be switched my changing the ParticleSystems Config member variable. Instead of changing each element separately. This would also explain why Config's member variables are all declared public, which i thought was BAD design! Shows what i know.

Not neccessarily. What I mean was more along the lines of... consider how ParticleSystem's constructor would look if ParticleSystem had, say, ten different settings. It might looks like this:

ParticleSystem::ParticleSystem(int setting0,int setting1,int setting2,int setting3,int setting4,int setting5,int setting6,int setting7,int setting8,int setting9,int setting10);

That's ugly, but not unheard of. It's especially annoying if you want everything but setting7 to be default values; you still have to pass those values. One way (among many) to alleviate this problem is to make the constructor look like

ParticleSystem::ParticleSystem(const ParticleSystem::Config &config);

then users can do

ParticleSystem::Config config; // Default construction; all values are defaults.config.setting7 = 144;ParticleSystem system(config);

As for public variables, the config class is essentially a POD (plain old data) type, and so public variables are generally okay. However, the issue is not, in general, that variables are public, but rather that they are accessible. If we made all the values private but provided get/set accessors for both, we've changed very little. From a language-agnostic design standpoint, the class design is just as bad -- we've gained the ability to more easily add checks, constraints and such, but we've also gained a lot more code meaning things are not as maintainable. Sometimes this is an even trade, sometimes not. However, since in this case the class is POD, it is acceptable. Not good, but it's probably not the worst offense in the subsystem.

Quote:
See i was thinking that it was written in a way to discourage creating Config objects outside of the ParticleSystem class. But what its actually saying is, you can creat them but i'm making you go through ParticleSystem::Config to make sure you understand its a config class for the particle system.

If the Config class were private, you couldn't create config objects outside the class. That is another common use for inner classes, but its not what's going on here. Here, all that's happening is that Config is being put in ParticleSystem's scope -- since you can't make a namespace with the same name as a class, the only other way to indicate that the Config class was for the particle system would be to call it ParticleSystemConfig.

Quote:Original post by JimmyDeemo

Quote:Original post by DaveConfig should have accessors to its members so that it can do any necessary validation when setting their values.

This is what i would have thought. I can only assume that on this occasion they were written as public for speed and simplicity over validation and encapsulation.

Making it public and invalidated doesn't make it simple and fast. If anything it adds complexity when someone who doesn't know the code comes to use it. They will go "hey i have a config class!", "What constraints are there on the inputs?". So they end up going and looking for documentation and asking people. Often there is no commenting or documentation.

If the accessors are validated then the programmer can look at them and get a picture of what is going on. If it is simple a POD class or struct then nothing is lost with accessors.

If the API is closed then the ParticleSystem::Config will be documented anyway. If it's not then the name gives away where you need to look. How is looking at the validations in the accessor methods any different to looking up the documentation?

(Edit) Removed irrelevant point.

Inner classes are another way of organising code that make sense in some situations.

Quote:Original post by Metorical
If the API is closed then the ParticleSystem::Config will be documented anyway. If it's not then the name gives away where you need to look. How is looking at the validations in the accessor methods any different to looking up the documentation?

Also the inner class doesn't need to have a public constructor so it's parent class can control creation and hand off to users.

Inner classes are another way of organising code that make sense in some situations.

Quote:Original post by DaveOften there is no commenting or documentation.