Implementation of warning storage

Question

I implemented type (UserRuleManager) that will apply list of rules to entity User. I need your code review and your advices.
Here is usage of that type

private void ProcessUser(User user) 
{
 if (!_userRuleManager.ApplyGeneralRules(user)) 
 {
 SetWarning();
 } else if (!_userRuleManager.ApplySpecificRules(user)) {
 SetWarning();
 } else {
 CreateUser(user);
 }
}
private void SetWarning() 
{
 foreach (var warningMessage in _userRuleManager.GetWarnings()) 
 {
 Warnings.Add(warningMessage);
 }
}

I have 2 types of rule: general and specific. First, I'm trying to apply general rules (ApplyGeneralRules) and if it is false then get list of warnings. The same for specific rules (ApplySpecificRules).
In implementation of UserRuleManager I have:

private list of warnings:

 private readonly List<string> _warnings;

private property

 private bool IsVerified 
 {
 get { return _warnings.Count == 0; }
 }

method to return list with warnings

public ICollection<string> GetWarnings() 
{
 return _warnings;
}

method for general rules

public bool ApplyGeneralRules(User user) 
{
 _warnings.Clear();
 ICollection<Rule> rules = _userRepository.GetActiveGeneralRules(user.CustomerId);
 foreach (Rule rule in rules.OrderBy(x => x.Order)) 
 { 
 if (RuleChecker.IsVerified(rule, user)) 
 {
 _warnings.Add(string.Format("General rule : {0}", rule.TypeId));
 }
 }
 return IsVerified;
}

and method for specific rules

public bool ApplySpecificRules(User user) 
{
 _warnings.Clear();
 var questions = GetQuestions(user);
 if (!questions.Any()) 
 {
 _warnings.Add(string.Format("No questions with code {0}", user.QuestionCodeId));
 return false;
 }
 foreach (var question in questions.OrderBy(q => q.Order)) 
 {
 var rules = _userRepository.GetSpecificRules(user.CustomerId, question.Id);
 foreach (var rule in rules) 
 {
 if (!RuleChecker.IsVerified(rule, user)) 
 {
 _warnings.Add(string.Format("Specific rule: {0} -> {1} -> {2}", question.TypeId, question.CodeId, rule.TypeId));
 }
 }
 }
 return IsVerified;
}

Could you please give me advice how can I improve or maybe implement more proper relation between this entities. What the pattern can I use here?
Thank you in advance and let me know if I should give more information

UPDATE

public static bool IsVerified(Rule rule, User user) {
RuleParameterValue daysParameter = rule.RuleParameterValues.SingleOrDefault(v => v.RuleParameterTypeId == Rule.Parameter.Days);
if (daysParameter == null) {
 return false;
}
int days = daysParameter.IntValue;
DateTime? lastLoginDateUtc = _userRepository.GetLastLoginDate(user.Id);
if (lastLoginDateUtc == null || lastLoginDateUtc.Value.AddDays(days) <= DateTime.UtcNow) {
 return true;
} else {
 return false;
}
}

• AlanT: Can you add the Rule and RuleChecker classes as well please. It is possible that we can factor out the RuleChecker by making the Rule do more. This would mean less changes if we wanted to add new rule shapes.
• Roman Marusyk: Rule is a simple model class. I've updated question but added only RuleChecker . Thanks for advice
• Mast: I have rolled back the last edit. Please see What to do when someone answers .
• Roman Marusyk: @Mast I know what should I do when someone answers, But this was not an answer. AlanT ask me to add some code to more understanding. In a result I updated my question. I used the edit link on my question to add additional information. What is the problem?

Answer 1

I think I got it :) I removed my previous answer as it was not enough SOLID (too much dependency on subtypes). Here is how it can be:

 var repository = new UserRepository();
 IRule<User> rule = new Validator<User>(
 new GeneralRules(repository),
 new SpecificRules(repository));
 var warnings = rule.Validate(new User());
 if(!warnings.Any())
 Console.WriteLine("Add user");
 else
 foreach (var warning in warnings)
 Console.WriteLine(warning);

Where:

 class Warning : IComparable<Warning>
 {
 public Warning(string message, int priority)
 {
 Message = message;
 Priority = priority;
 }
 public override string ToString() => Message;
 public int CompareTo(Warning other) =>
 other.Priority.CompareTo(Priority);
 string Message { get; }
 int Priority { get; }
 }

And:

 interface IRule<T>
 {
 IEnumerable<Warning> Validate(T subject);
 }

And:

 interface IRuleSource<T>
 {
 IEnumerable<IRule<T>> Query(T subject);
 }

And:

 class Validator<T> : IRule<T>
 {
 public Validator(params IRuleSource<T>[] sources)
 {
 Sources = sources;
 }
 public IEnumerable<Warning> Validate(T subject) =>
 Sources
 .Select(s => s
 .Query(subject)
 .SelectMany(r => r.Validate(subject))
 .OrderBy(w => w))
 .FirstOrDefault(sw => sw.Any()) ?? 
 Enumerable.Empty<Warning>();
 IEnumerable<IRuleSource<T>> Sources { get; }
 }
}

And:

 class User
 {
 public string Name { get; set; }
 public int CustomerId { get; set; }
 public int QuestionCodeId { get; set; }
 }
 class Question
 {
 public int Id { get; set; }
 }
 interface IUserRepository
 {
 IEnumerable<IRule<User>> GetActiveGeneralRules(int customerId);
 IEnumerable<IRule<User>> GetSpecificRules(int customerId, int questionId);
 IEnumerable<Question> GetQuestions(int questionCodeId);
 }

And:

 class GeneralRules : IRuleSource<User>
 {
 public GeneralRules(IUserRepository repository)
 {
 Repository = repository;
 }
 public IEnumerable<IRule<User>> Query(User subject) =>
 Repository.GetActiveGeneralRules(subject.CustomerId);
 IUserRepository Repository { get; }
 }

And:

 class SpecificRules : IRuleSource<User>
 {
 static IRule<User> MissingsQuestions { get; } = new MissingsQuestionsRule();
 public SpecificRules(IUserRepository repository)
 {
 Repository = repository;
 }
 public IEnumerable<IRule<User>> Query(User subject) =>
 Repository.GetQuestions(subject.QuestionCodeId)
 .SelectMany(q => Repository.GetSpecificRules(subject.CustomerId, q.Id))
 .DefaultIfEmpty(MissingsQuestions);
 IUserRepository Repository { get; }
 }

Where:

 class MissingsQuestionsRule : IRule<User>
 {
 public IEnumerable<Warning> Validate(User subject)
 {
 yield return new Warning($"No questions with code {subject.QuestionCodeId}", 1000);
 }
 }

Answer 2

The only criticism with the code is that your business rules are implemented in the third-party class UserRuleManager. This is OK if you are developing a a "rule system", but otherwise you should encapsulate the rules within the user class if they are about a user; the user factory if they are about how to create users; and the user repository (or data access class) if they are about how to persist and retrieve user objects.

Also, be wary if your class ends with "Manager"; it signals that the class will be responsible of too many things -- a God class.

Answer 3

You have a bug in the ApplyGeneralRules() method. You are adding warnings if RuleChecker.IsVerified() is returning true.

Answer 4

The first thing that hit me was a seeming chicken and egg issue (though it is probably a naming thing). We call ProcessUser() and if there are no warnings we then CreateUser(), but the parameter for both of these is a User. If we already have a user, what does CreateUser do - if it doesn't create the user, should that method be called something else?

The next thing for me was that we only have warnings and that all warnings are equal - is this correct, or do some rules have more importance than others?

Even if all rules are equal, by storing the failures as strings we lose a lot of information. We have no specific message from the failed rule and if we want to find out details of the failed rule we would need to parse the warning to find the TypeId for the Rule (and for Question details in the specific rules) and then look them up somehow.

Another approach would be to store the failed rules in the message

class RuleResult
{
 public RuleResult(IUserRule rule, string message)
 {
 Rule = rule
 Message = message;
 }
 public IUserRule Rule { get; }
 public string Message { get; }
}
abstract class FailedRuleMessage
{
 public FailedRuleMessage(RuleResult result)
 {
 Result = result;
 }
 public RuleResult Result {get; }
 public IUserRule Rule => Result.Rule;
} 
class GeneralRuleMessage : FailedRuleMessage
{
 public GeneralRuleMessage(RuleResult result) : base(result)
 {
 }
 public override string ToString()
 {
 return $"General rule : {Rule.TypeId}";
 }
}
class SpecificRuleMessage : FailedRuleMessage
{
 public SpecificRuleMessage(RuleResult result, IQuestion question) 
 : base(result)
 {
 Question = question;
 }
 public IQuestion Question { get; }
 public override string ToString()
 {
 return $"Specific rule: {Question.TypeId} -> {Question.CodeId} -> {Rule.TypeId}";
 }
}

This gives us the current functionality, but also allows us to add more details to messages if desired.

Note: I have created interfaces a few of the types. This is pretty much a Matter Of Personal Preference (MOPP). I find it useful for unit testing and expansion but not everyone would agree.

interface IUser
{
 string Id { get; }
}
interface IQuestion
{
 string TypeId { get; }
 string CodeId { get; }
}
interface IUserRule
{
 int Order { get; }
 string TypeId { get; }
 RuleResult Verify(IUser user);
}

At the moment, we have a RuleChecker that verifies a rule against a user. A problem with this is that unless all the logic for verifying the rule resides in the rule - i.e. the checker has no knowledge about either the Rule or User - if we add new rule types we would need to make changes to the RuleChecker.

If all the logic does reside in the Rule then we shouldn't need a checker, we can simply invoke the rule directly.

Having a Boolean result from the rule is limiting. We know only pass/fail. There is no degree of failure and no details on why it failed. Even if this is perfectly valid right now it may not always be so.

Adding a RuleResult allows more detail. A rule now passes if it returns no RuleResult. We can verify a set of general rules as follows

class UserRuleManager
{
 private IUserRuleProvider _ruleProvider;
 private bool IsVerified
 {
 get { return !Results.Any(); }
 }
 public IEnumerable<FailedRuleMessage> Results { get; private set; }
 private bool ApplyGeneralRules(IUser user)
 {
 Results = _ruleProvider.GetRules(user)
 .Select(r => r.Verify(user))
 .Where(res => res != null)
 .Select(res => new GeneralRuleMessage(res));
 return IsVerified;
 } 
 //...
}

There should be a more elegant way to do the specific rules (with, perhaps a NullQuestion returned instead of an empty list and an always failing rule that is fetched for the NullQuestion) but I don't have time at the moment.

• Dmitry Nogin: I would say that: 1) We do not need to know about General vs. Specific – it is basically some groups of rules to be applied consequently, till the first match. No need to know type, this aspect can be abstracted and be ready to support more groups. 2) Consumer needs to ask a simple question – do we have any Warnings or not. The proposed design requires client code to have dependency on all the classes: RuleResult, FailedRuleMessage and subtypes, which is not necessary. 3) RuleResult.Rule property violates encapsulation and creates non-reasonable bidirectional association.
• AlanT: I don't disagree about the general and specific, they are just rules and reading the ProcessMethod we are always verifying all rules. The only glitch is that per the current code we want to know about the question on a per rule basis - this may be a false requirement but it is in the original code. Getting all the rules (general and specific) at once and verifying them doesn't give access to the question for each rule. ATM, the client only needs reference to the RuleResult not sub-types. Rule and Question could/should be protected. I'd disagree on the reasonableness of the association.
• Dmitry Nogin: Actually we do not have questions on a per rule basis; we have rules on a per question basis. It makes unified IRuleSource possible (have a look at my new answer - it is a way shorter now :)
• AlanT: Yes, but we need to report on the failed rules including the question information. I can't see that in your answer though I do like it. Unless the rules have an association with the question (which I was trying to avoid) I don't see how Query in SpecificRules gives me that information. As I said it may not be needed but I was trying to recreate the original code as closely as possible.