5
\$\begingroup\$

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;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ which version of C# do you use? \$\endgroup\$
    – Snowbear
    May 28 '11 at 20:21
  • \$\begingroup\$ I use .NET 3.5 SP1 \$\endgroup\$
    – user4391
    May 29 '11 at 3:56
4
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Hello Sgrassie, I thought about using XML serialization for this as I am using it in other areas of my project. To the best of my knowledge, XML serialization will group the tags by name and not necessarily by the order that they appear in the file. I need the checks to be performed in the order that they appear in the XML file. I also looked over some XPath tutorials and did not see any distinct advantage of using XPath over the foreach loop as I would still need to get the node name and attributes. Perhaps you could elaborate more on what you were thinking. \$\endgroup\$
    – user4391
    May 31 '11 at 13:28
  • \$\begingroup\$ Without seeing an example of the xml it's a little difficult to offer further advice. Are they always the same checks? Are they always in the same order? If the order of the elements is changeable then I would expect some piece of data which tells me what the order is. \$\endgroup\$ May 31 '11 at 14:27
0
\$\begingroup\$

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.

\$\endgroup\$
7
  • 4
    \$\begingroup\$ IsDomainGroupExists doesn't sound like correct English \$\endgroup\$
    – Snowbear
    May 28 '11 at 20:22
  • \$\begingroup\$ @Snowbear JIM-compiler :Agree with your BUT, This is the common in object oriented programming for starting the name of a function with "Is" if they do validation. \$\endgroup\$
    – Jhaliya
    May 29 '11 at 6:08
  • \$\begingroup\$ Perhaps Does would be the best word for this situation. \$\endgroup\$
    – user4391
    May 29 '11 at 20:37
  • \$\begingroup\$ IsDomainGroup would read better. \$\endgroup\$ May 31 '11 at 10:01
  • 1
    \$\begingroup\$ I see no problem with DomainGroupExists, where IsDomainGroup would be misleading. You're not checking to see if it is a domain group, you're checking to see if it exists, so the name describes exactly what it's doing. I would say the convention of describing what you're doing outweighs the "convention" of starting a validation function with "Is". \$\endgroup\$
    – Joel C
    May 31 '11 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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