Proper way to enforce business rules in property setter

Question

Let's say an application has an Employer object has been created by a user and the program is prompting the user to enter in a property called EmployerId. In this example the application won't let the user enter in an invalid EmployeeId. I have mocked up the following solution to solve the problem.

public int EmployerId
        {
            get { return employerId; }
            set
            {

                EmployerId = SetEmployerId();
            }
        }

        public int SetEmployerId()
        {
            int id = 0;
            Console.Write("Enter employer ID: ");
            id = Convert.ToInt32(Console.ReadLine());
            while (id < 5 || id > 10) //silly logic for illustrative purposes
            {
                Console.Write("Invalid id, enter another id: ");
                id = Convert.ToInt32(Console.ReadLine());
            }
            return this.employerId = id;
        }

this keeps any heavy lifting out of the setter of the property and delegates that responsibility to the SetEmployerId method.

Would this be an acceptable solution in a production setting? Is there any way this could be improved upon, or any pitfalls this might lead to later down the road when the application isn't such a contrived example? (besides the fact that the user isn't aware of what valid input is).

Answer 1

I think the better question is should an outside class be able to modify EmployerId directly.

Most often times methods that create mutation should exposed as verbs so something like ChangeEmployer(Employer newEmployer) and so on. Doing it that way make it more explicit and allows you to more easily raise domain events. In this case the set method would be private so that only the owning class can call it.

That said, any change of EmployerId should be verified in the setter that way the logic is only in one place and not strewn about multiple methods.

Karl Anderson's answer makes a very good point, that putting business logic in a setter like that prevents returning a non-exceptional error. This is true and should be taken into consideration before using property setters.

He also makes a good point about validation objects, your aggregate entities may only reference each other by Id as such having a separate business validation object for validating those Ids may be an excellent choice. You can reuse the validator in multiple places but at the end of the day the only place that matters is inside the entity, as that is the only place that must always be consistent.

public class Employee
{
     private EmployerId employerId;

     public Employee(EmployerId id /* other params such as name etc */)
     {
         var employerSetResult = this.SetEmployerId(id);
         if(!result.Success)
            throw new ArgumentException("id", "id is invalid");
     }

     // this is a separate method because you will need to set employerId
     // from multiple locations and should only ever call SetEmployerId 
     // internally
     public Result ChangeEmployer(EmployerId id)
     {
          var result = this.SetEmployerId(id);
          if(result.Success)
             DomainEventPublisher.Publish(
               new EmployeeEmployerChanged(id, this.id));
          return result;
     }

     private Result SetEmployerId(Employer id)
     {
          var result = EmployerIdValidator.Validate(id);
          if(result.Success)
             this.employerId = id;
          return result;
     }
}

public static class EmployerIdValidator
{
    public static Result Validate(EmployerId id)
    {
        if(id < 5)
           return new Result(success: false, new ValidationResult("Id too low"));
        else if (id > 10)
           return new Result(success: false, new ValidationResult("Id too high"));
        return new Result(success:true);
    }
}

public class Result
{
     public bool Success {get {return this.success;}}
     public IEnumerable<ValidationResult> ValidationResults 
     { 
        get{ return this.validationResults; }
     } 
}

Answer 2

I like approaches that Karl and Mgetz talked about. Just wanted that if you want to use setters and getters and to separate your bussiness logic from presentation you doomed to use Exceptions and your code coud look like this:

public class WrongEmployeeIDException : Exception
        {
            public WrongEmployeeIDException(string message) : base(message) { }
        }
 public class Employee
        {
            private int employerId;
            public int EmployerId
            {
                get { return employerId; }
                set
                {
                    if (value < 5 || value > 10)
                    {
                        throw new WrongEmployeeIDException("Invalid id");
                    }

                    employerId = value;
                }
            }
        }


        public void Main()
        {
            int id;
            string input;
            bool isSetted = false;
            Employee employee = new Employee();

            while (!isSetted)
            {
                Console.Write("Enter employer ID: ");
                try
                {
                    input = Console.ReadLine();
                    id = Convert.ToInt32(input);
                    employee.EmployerId = id;
                    isSetted = true;
                }
                catch (WrongEmployeeIDException ex)
                {
                    Console.WriteLine(ex.Message);
                    //not satisfied to bussiness rules
                }
                catch (FormatException ex)
                {
                    Console.WriteLine(ex.Message);
                    //Convert.ToInt32 thrown exception
                }
                catch
                {
                    //something more bad happend
                }
            }
        }

But it's not recommended aproach because validation logic will execute more smoothly and faster with Magetz's solution. It's something middle between your solusion and Magetz's.

Answer 3

One of the downsides of your approach is the inability to return "error" information about why the business rule failed. In your example you are directing output to the Console, which does the calling code no good.

I would recommend building a set of business rule classes that can be applied to your business objects and then processed upon an event (i.e. the presentation layer calls Validate on the business object) and then a collection (i.e. List<string> of errors) can be returned and used by the calling code (i.e. presentation layer).