C# Design – Database Calls in Constructor vs Method

cclean codedesignobject-orientedunit testing

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

Best Answer

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);
}
Related Topic