Is this a correct way to use nested classes to keep track of data?

Question

I have some instruments. Each instrument should have a name (string) and description (string).

Each instrument can have some setups, which these setups are each a series of commands(string[] or List).

I want this class to be flexible so I can easily add or remove a whole instrument, or add or remove setups for any instrument that is existing in the class.

At the end, I want to save this class in properties of my assembly so each time the program starts it can be loaded. I am also thinking about serializing it and save it on disk for moving to another copy of the software.

So please let me know what you think about this approach and if you think this can be improved or even I should consider going with another way please let me know!

namespace MeasurementSuite
{
    /// <summary>
    /// Setups for all instruments
    /// </summary>
    public class Setups
    {
        /// <summary>
        /// An instrument
        /// </summary>
        public class Instrument
        {
            /// <summary>
            /// Name of the instrument
            /// </summary>
            public string Name { get; set; }
            /// <summary>
            /// Info about the instrument
            /// </summary>
            public string Info { get; set; }
            /// <summary>
            /// List of existing setups
            /// </summary>
            public List<Setup> SetupList { get; set; }

            /// <summary>
            /// Adds a setup to the instrument
            /// </summary>
            /// <param name="name"></param>
            /// <param name="commands"></param>
            private void AddSetup(string name, string[] commands)
            {
                this.SetupList.Add(new Setup(name, commands));
            }

            /// <summary>
            /// Removes a setup from the instrument
            /// </summary>
            /// <param name="name">Name of the setup</param>
            public void RemoveSetup(string name)
            {
                foreach (Setup setup in SetupList)
                {
                    if (setup.Name == name)
                    {
                        SetupList.Remove(setup);
                    }
                }
            }        
        }

        /// <summary>
        /// Setup for a instrument
        /// </summary>
        public class Setup
        {
            /// <summary>
            /// Name of the setup
            /// </summary>
            public string Name { get; set; }
            /// <summary>
            /// Command sets for setup
            /// </summary>
            public string[] CommandSet { get; set; }

            /// <summary>
            /// Constructor
            /// </summary>
            /// <param name="name">Setup Name</param>
            /// <param name="commands">Setup Command sets</param>
            public Setup(string name, string[] commands)
            {
                this.Name = name;
                this.CommandSet = commands;
            }
        } 
    }
}

Answer 1

Nested Classes to create private classes is NOT bad. In fact that is a good thing to make the classes not visible to outside world. But for this, they should must be private.

However, as per your above code, these classes are public so they become visible to rest of the application. Such things has no special value except that class hierarchy really have special connotations.

Such a thing is good only if they are followed based on a well established model from the real world, but it can become mess if this is done for programmer's convenience.

The other non-functional but very critical difference is, that Nested Classes declaration makes the Code very unreadable often because of a lot of hierarchy. To help that you should declare methods there, but implement them outside in the .cpp file separately.

Dipan.

Answer 2

Here's the harsh truth: It is probably not the right way to store it.

On the bright side: Nobody knows how to get it right until he has seen all possible use cases of that class. It looks ok, but you would have to see it 'in action' to judge it properly.

For now just go with it and refactor if you need to. The right design will probably emerge if you stick to the SOLID principles and constantly try to improve it. Keep this copy as a reference point, then come back in a couple of weeks and see which changes you made and if you could have predicted them.

The closest you'll ever get to getting it right is designing the whole system upfront, which raises a lot of other problems. This class itself without any context could be right, but also could be terribly wrong.

---

This is the programmers.stackexchange answer. If you want more details try codereview.stackexchange.