Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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;
            }
        } 
    }
}
share|improve this question
1  
AddSetup is currently private, so you can't do something like: Setups.Instrument inst = new Setups.Instrument(); and then inst.AddSetup("Setup1", new string[] {"Execute"}); Also, you never instantiate the SetupList so trying to access it throws exceptions. maybe add it in a default constructor public Instrument() { SetupList = new List<Setup>();} –  Dylan Yaga Nov 3 '11 at 11:06
    
@Sean, please change the question title so it reflects the question intent- Thanks. –  Emmad Kareem Nov 3 '11 at 11:14
add comment

2 Answers

up vote 1 down vote accepted

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.