8

Take the following instance for example: CreditCardApplication class

 public class CreditCardApplication
    {
        public int Id { get; set; }
        public string FirstName { get; set; }
        public string LastName { get; set; }
        public int Age { get; set; }
        public decimal GrossAnnualIncome { get; set; }
        public string FrequentFlyerNumber { get; set; } 
    }

And there is a CreditCardApplicationEvaluator class

public class CreditCardApplicationEvaluator
    {
        private const int AutoReferralMaxAge = 20;
        private const int HighIncomeThreshhold = 100_000;
        private const int LowIncomeThreshhold = 20_000;

        public CreditCardApplicationDecision Evaluate(CreditCardApplication application)
        {
            if (application.GrossAnnualIncome >= HighIncomeThreshhold)
            {
                return CreditCardApplicationDecision.AutoAccepted;
            }

            if (application.Age <= AutoReferralMaxAge)
            {
                return CreditCardApplicationDecision.ReferredToHuman;
            }

            if (application.GrossAnnualIncome < LowIncomeThreshhold)
            {
                return CreditCardApplicationDecision.AutoDeclined;
            }

            return CreditCardApplicationDecision.ReferredToHuman;
        }       
    }

CreditCardApplicationDecision is an enum.

Now as you can see there are certain private const defined inside CreditCardApplicationEvaluator class. In a real world application these values will most probably come from a database.

So the question is where do I make the database call to set these constants? Inside the constructor? Or within Evaluate method?

Should I create another class Thresholds with the properties such as AutoReferralMaxAgeetc and pass it on to Evaluate method as follows:

static void Main(string[] args)
{
   //CreditCardApplication instance creation code goes here... 

   Thresholds thresholds =new Thresholds();
   thresholds = _unitOfWork.CreditcardRepository.GetThresholds();
   CreditCardApplicationEvaluator eval=new CreditCardApplicationEvaluator();
  var decision = eval.Evaluate(creditCardApplication,thresholds);
}

Or should I just place unitOfWork.CreditcardRepository.GetThresholds() inside Evaluate method of CreditCardApplicationEvaluator class, or perhaps it's constructor?


Calling database inside a constructor is probably a bad idea as pointed out by @Greg. So now what is left unanswered is that should I :

  1. Have unitOfWork.CreditcardRepository.GetThresholds() inside main()
  2. or within Evaluate() method of CreditCardApplicationEvaluator class
4
  • 1
    Possible duplicate of Is there ever a reason to do all an object's work in a constructor?
    – gnat
    Jun 5, 2019 at 13:40
  • @gnat I beg to differ. I don't think the mentioned post could have answered my question.My question does not ask for a situation when using constructor is good or bad idea. But it asks whether in a given situation, it will make sense? If not what is the most sensible approach out of the other two mentioned ways. Jun 5, 2019 at 14:27
  • accepted answer over there states the same as the answer you accepted here, "constructors should do a minimum of work", and other top voted answers explain the same point in details (I particularly like the way it is laid out in 3rd top voted answer over there)
    – gnat
    Jun 5, 2019 at 14:32
  • @gnat That is only a part of the question that has been answered over there. Constructor should be used for minimal work. Sure. But I accepted the answer only after Greg edited his answer to addressed.. Should I : 1. Have unitOfWork.CreditcardRepository.GetThresholds() inside main() 2. or within Evaluate() method of CreditCardApplicationEvaluator class Jun 5, 2019 at 14:38

2 Answers 2

11

Constructors should not do work. The initialization of a new object should happen very quickly, and making database calls, or interacting with any resource outside of the current process, can take considerably longer.

Instead, your constructor should either require the data from the database as separate arguments, so that the database calls are made before creating the new object, or move that logic into an instance method.

Constructors should be limited to allocating new memory for the new object (like initializing empty collections or assigning default values to required fields) and ensuring the object is in a valid state upon first being created.

Anything beyond that belongs in an instance method, or arguments passed into the constructor.

To be specific to your use case, the three "const" integers should be arguments to the constructor. Furthermore, the income threshold is actually a good candidate for a new class, because you don't want a high threshold of 20,000 and a low threshold of 100,000.

public class IncomeThreshhold
{
    public int Low { get; }
    public int High { get; }

    public IncomeThreshhold(int low, int high)
    {
        if (low < 0)
            throw new ArgumentOutOfRangeException();

        if (high < 0)
            throw new ArgumentOutOfRangeException();

        if (high < low)
            throw new ArgumentOutOfRangeException();

        Low = low;
        High = high;
    }
}

Then your CreditCardApplicationEvaluator class becomes a little easier to construct with valid data:

public class CreditCardApplicationEvaluator
{
    private int AutoReferralMaxAge { get; }
    private IncomeThreshhold IncomeThreshhold { get; }

    public CreditCardApplicationEvaluator(int autoReferralMaxAge, IncomeThreshhold incomeThreshhold)
    {
        if (autoReferralMaxAge < someValueProbablyDrivenByRegulations)
            throw new ArgumentOutOfRangeException();

        AutoReferralMaxAge = autoReferralMaxAge;
        IncomeThreshhold = incomeThreshhold;
    }

    public CreditCardApplicationDecision Evaluate(CreditCardApplication application)
    {
        if (application.GrossAnnualIncome >= IncomeThreshhold.High)
        {
            return CreditCardApplicationDecision.AutoAccepted;
        }

        if (application.Age <= AutoReferralMaxAge)
        {
            return CreditCardApplicationDecision.ReferredToHuman;
        }

        if (application.GrossAnnualIncome < IncomeThreshhold.Low)
        {
            return CreditCardApplicationDecision.AutoDeclined;
        }

        return CreditCardApplicationDecision.ReferredToHuman;
    }
}

The initialization of these objects in conjunction with a unit of work or repository ends up like this:

static void Main(string[] args)
{
    var incomeThreshhold = unitOfWork.CreditcardRepository.GetThreshold(...);
    var autoReferralMaxAge = unitOfWork.CreditcardRepository.GetAutoReferralMaxAge(...);
    var evaluator = new CreditCardApplicationEvaluator(autoReferralMaxAge, incomeThreshhold);
    var application  unitOfWork.CreditcardRepository.GetApplication(...);
    var decision = evaluator.Evaluate(application);
}
4
  • What would you rather do? Have unitOfWork.CreditcardRepository.GetThresholds() inside main() or within Evaluate() method, in the example that I have posted? Jun 5, 2019 at 11:55
  • @Arbaaz: Updated my answer in response to your comment. Jun 5, 2019 at 12:04
  • That was very helpful @Greg. I have just started on working on my ability to write cleaner , testable code. Just one more question, if you don't mind. Supposing I have one single table that has criteria for CreditCard Application Evaluation ie all if the three AutoReferralMaxAge, HighIncomeThreshhold, LowIncomeThreshhold , would it be a bad idea to pass this POCO object fetched from database via entity framework directly to Evaluate method? I could add if (low < 0) throw new ArgumentOutOfRangeException(); etc within POCO itself if i have to. Would that be terrible? Jun 5, 2019 at 12:23
  • 1
    @Arbaaz: No. If the age and income thresholds are a single logical entity, then a class that encapsulates all three values is actually the right way to go. You can modify the CreditCardApplicationEvaluator class to accept a collection of these thresholds if you want to increase the flexibility of the evaluator. Jun 5, 2019 at 12:31
1

When you instantiate the object CreditCardApplicationEvaluator, it needs to be ready to evaluate applications. We must think of the failure case where you can't pull this data from the database. So if you put the db call for those constants in the constructor, it might never return, so you will never instantiate your object or it will fail.

Also another reason to not put in the constructor is every single instantiation you are hitting the database. I assume the data is not updated in the db this frequently?

Also your db calls should be on the periphery and not tightly coupled to the business logic. What if I want to change where I get these constants from? Maybe I eventually want some from one data source and some from another? The evaluator class shouldn't be dependent upon some database.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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