patterncsharpMinor
Improving the design of this program.
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))
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
One possibility is to fill a dictionary:
where, for example,
Then your big switch statement at the end just becomes:
The delegates are still in one place, but the logic is clearer in the
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.