HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Improving the design of this program.

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisthedesignprogramimproving

Problem

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

///
/// Initializes a new instance of the class.
///
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;
}

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

if (string.IsNullOrEmpty(groupName))

Solution

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 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.

Code Snippets

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);
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;
    }
}
AllDelegates theDelegate = typeDictionary[node.Name];
var req = theDelegate(node, onPass, onFail);

Context

StackExchange Code Review Q#2675, answer score: 4

Revisions (0)

No revisions yet.