Improving the design of this program.

I am trying to design a program that will read a XML file and based on the tag name will perform certain requirements checks. I feel that there is probably a better way of doing this and any comments or suggestions are welcomed.

Thank you.

public enum AccountCheckType
{
    UserIsMemberOfGroup,
    DomainUserExists,
    DomainGroupExists
}


public class AccountManager
{

    public bool DomainGroupExists(string groupName)
    {
        //Todo: Implement this
        return false;
    }

    public bool DomainUserExists(string userName)
    {
        //Todo: Implement this
        return false;
    }

    public bool UserIsMemberOfGroup(string userName,string groupName)
    {
        //Todo: Implement this
        return false;
    }
}


public class AccountRequirement:Requirement
{
    #region Declarations

    private readonly AccountManager manager = new AccountManager();
    private readonly AccountCheckType checkType;
    private string accountName;
    private string groupName;

    #endregion

    #region Constructor/Deconstructor

    /// <summary>
    /// Initializes a new instance of the <see cref="AccountRequirement"/> class.
    /// </summary>
    public AccountRequirement(string accountName,AccountCheckType checkType)
    {
        if(string.IsNullOrEmpty(accountName))
        {
            throw new ArgumentException("Account name cannot be null or empty.");
        }

        if(checkType==AccountCheckType.UserIsMemberOfGroup)
        {
            throw new ArgumentException("Checktype cannot be equal to UserIsMemberOfGroup");
        }

        this.accountName = accountName;
        this.checkType = checkType;
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="AccountRequirement"/> class.
    /// </summary>
    /// <param name="userName">Name of the user.</param>
    /// <param name="groupName">Name of the group.</param>
    public AccountRequirement(string userName,string groupName)
    {
        if(string.IsNullOrEmpty(userName))
        {
            throw new ArgumentException("Username cannot be null or empty.");
        }

        if (string.IsNullOrEmpty(groupName))
        {
            throw new ArgumentException("Groupname cannot be null or empty.");
        }

        this.checkType = AccountCheckType.UserIsMemberOfGroup;
    }

    #endregion

    public override RequirementStatus PerformCheck()
    {
        switch (checkType)
        {
            case AccountCheckType.DomainGroupExists:
                CurrentStatus = manager.DomainGroupExists(groupName) ? OnPass : OnFail;
                return CurrentStatus;
            case AccountCheckType.DomainUserExists:
                CurrentStatus = manager.DomainUserExists(accountName) ? OnPass : OnFail;
                return CurrentStatus;
            case AccountCheckType.UserIsMemberOfGroup:
                CurrentStatus = manager.UserIsMemberOfGroup(accountName, groupName) ? OnPass : OnFail;
                return CurrentStatus;
            default:
                throw new Exception("Unknown checkType in account requirement");
        }
    }
}

public enum RegistryCheckType
{
    RegistryKeyExists,
    RegistryValueExists
}

public class RegistryRequirement:Requirement
{
    #region Declarations

    private RegistryManager manager = new RegistryManager();
    private RegistryCheckType checkType;
    private string key;
    private string subKey;
    private string value;

    #endregion

    #region Constructor/Deconstructor

    /// <summary>
    /// Initializes a new instance of the <see cref="RegistryRequirement"/> class.
    /// </summary>
    public RegistryRequirement(string key,string subKey)
    {
        if(string.IsNullOrEmpty(key))
        {
            throw new ArgumentException("Key cannot be null or empty.");
        }

        if (string.IsNullOrEmpty(subKey))
        {
            throw new ArgumentException("Subkey cannot be null or empty.");
        }

        this.key = key;
        this.subKey = subKey;
        this.checkType = RegistryCheckType.RegistryKeyExists;
    }

    public RegistryRequirement(string key, string subKey,string value)
    {
        if (string.IsNullOrEmpty(key))
        {
            throw new ArgumentException("Key cannot be null or empty.");
        }

        if (string.IsNullOrEmpty(subKey))
        {
            throw new ArgumentException("Subkey cannot be null or empty.");
        }

        if (string.IsNullOrEmpty(value))
        {
            throw new ArgumentException("Value cannot be null or empty.");
        }

        this.key = key;
        this.subKey = subKey;
        this.value = value;
        this.checkType = RegistryCheckType.RegistryValueExists;
    }

    #endregion


    public override RequirementStatus PerformCheck()
    {
        switch (checkType)
        {
            case RegistryCheckType.RegistryKeyExists:
                CurrentStatus = manager.RegistryKeyExists(key, subKey) ? OnPass : OnFail;
                return CurrentStatus;
            case RegistryCheckType.RegistryValueExists:
                CurrentStatus = manager.RegistryValueExists(key, subKey, value) ? OnPass : OnFail;
                return CurrentStatus;
            default:
                throw new Exception("Unknown checkType in registry requirement");
        }
    }
}

public abstract class Requirement
{
    #region Constructor/Deconstructor

    /// <summary>
    /// Initializes a new instance of the <see cref="Requirement"/> class.
    /// </summary>
    public Requirement()
    {
    }

    #endregion

    #region Properties

    public RequirementStatus OnPass { get; set; }
    public RequirementStatus OnFail { get; set; }
    public RequirementStatus CurrentStatus { get; protected set; }

    #endregion

    public abstract RequirementStatus PerformCheck();
}

public class RegistryManager
{
    #region Declarations



    #endregion

    #region Constructor/Deconstructor

    /// <summary>
    /// Initializes a new instance of the <see cref="RegistryManager"/> class.
    /// </summary>
    public RegistryManager()
    {
    }

    #endregion

    #region Properties



    #endregion

    public bool RegistryKeyExists(string key,string subKey)
    {
        //Todo: Implement this
        return true;
    }

    public bool RegistryValueExists(string key,string subKey,string value)
    {
        //Todo: Implement this
        return true;
    }
}

public class RequirementFileReader
{
    #region Declarations

    private XmlDocument xmlDocument=new XmlDocument();
    private string filePath;

    #endregion

    #region Constructor/Deconstructor

    /// <summary>
    /// Initializes a new instance of the <see cref="RequirementFileReader"/> class.
    /// </summary>
    public RequirementFileReader(string filePath)
    {
        if(string.IsNullOrEmpty(filePath))
        {
            throw new ArgumentException("File path cannot be null or empty.");
        }

        this.filePath = filePath;
    }

    #endregion

    #region Properties



    #endregion

    public IEnumerable<Requirement> GetRequirements()
    {
        var requirements=new List<Requirement>();
        xmlDocument.Load(filePath);
        var rootNode = xmlDocument.DocumentElement;
        if(rootNode==null)
        {
            throw new Exception("Not a valid xml file.");
        }
        foreach (XmlNode node in rootNode.ChildNodes)
        {
            var onPass = (RequirementStatus) Enum.Parse(typeof (RequirementStatus), node.Attributes["onpass"].Value);
            var onFail = (RequirementStatus) Enum.Parse(typeof (RequirementStatus), node.Attributes["onfail"].Value);

            Requirement req = null;
            switch (node.Name)
            {
                case "UserExists":
                    var user=node.Attributes["username"].Value;
                    req = new AccountRequirement(user, AccountCheckType.DomainUserExists)
                              {OnPass = onPass, OnFail = onFail};
                    break;
                case "GroupExists":
                    var group = node.Attributes["groupname"].Value;
                    req = new AccountRequirement(group, AccountCheckType.DomainGroupExists) { OnPass = onPass, OnFail = onFail };
                    break;
                case "RegKeyExists":
                    var key=node.Attributes["key"].Value;
                    var subkey=node.Attributes["subkey"].Value;
                    req = new RegistryRequirement(key, subkey);
                    break;
                case "RegValueExists":
                    var key1 = node.Attributes["key"].Value;
                    var subkey1=node.Attributes["subkey"].Value;
                    var value1=node.Attributes["value"].Value;
                    req = new RegistryRequirement(key1, subkey1, value1);
                    break;
                default:
                    throw new Exception("Invalid xml tag found.");
            }
            requirements.Add(req);
        }
        return requirements;
    }
}

On thing that worries me is the number of switch statements you have in the code. Some OO practitioners see it as a code smell.

One possibility is to fill a dictionary:

public delegate Requirement AllDelegates(XmlNode node);
Dictionary<string,delegate> typeDictionary;

typeDictionary.Add("UserExists", RequirementDelegates.UserDelegate);
typeDictionary.Add("GroupExists", RequirementDelegates.GroupDelegate);
typeDictionary.Add("RegKeyExists", RequirementDelegates.RegKeyDelegate);
typeDictionary.Add("RegValueExists", RequirementDelegates.RegValueDelegate);

where, for example, UserDelegate is a static member of the RequirementDelegates class:

class RequirementDelegates
{
    public static Requirement UserDelegate(XmlNode node, 
                                           RequirementStatus onPass, 
                                           RequirementStatus onFail )
    {
        var user = node.Attributes["username"].Value;
        var req  = new AccountRequirement(user, AccountCheckType.DomainUserExists)
                          {OnPass = onPass, OnFail = onFail};
        return req;
    }
}

Then your big switch statement at the end just becomes:

AllDelegates theDelegate = typeDictionary[node.Name];
var req = theDelegate(node, onPass, onFail);

The delegates are still in one place, but the logic is clearer in the GetRequirements method.

Whilst the code you have posted could be refactored and improved, I personally think you are trying to solve the problem in the wrong way.

If all you want is to pull certain bits of information out of certain elements, then I would definitely use XPath to achieve this. You could easily pull out the data from the elements you need to initialize your other classes, without all the cruft of the foreach and switch in the GetRequirements method.

Alternatively, you could generate an XSD (if there isn’t already one) for the XML you are consuming using XSD.exe, and then generate C# classes from the XSD.You’d probably want to tweak the XSD so that the various elements require they contain text or attributes. Then you could use the XSD to quickly validate the XML document and use the C# classes to serialise the XML into objects which you could then use to do further validation checks and initialise your other objects.

You program seem me a good design, it would be great and also increase the program readability, if you change the function name which perform checks.

They should start with Is

for example —> your function name DomainGroupExists could be change to IsDomainGroupExists … this could be applied to all your checks functions.