Do db calls in constructors lead to more DRY code?

Question

It recently came to my attention that its best practice to avoid database calls in constructors. I feel like this means you end up repeating unnecessary code, thus the code is less DRY?

For example, this approach means the list of available courses is available in the Semester object and the db call exists once.

public class Semester{
     public int SelectedCourse {get; set;}
     public IEnumerable<Course> AvailableCourses {get; set;}
     
     public Semester()
     {
      AvailableCourses = dbAcessor.GetAllCourses();
     }
}

public class Course{
     public string Name {get; set;}
     public int Id {get; set;}
}


var semester = new Semester();
var semester2 = new Semester();

Whereas this approach would require the db call to exist every time the Semester object was initialized.

public class Semester{
     public int SelectedCourse {get; set;}
     public IEnumerable<Course> AvailableCourses {get; set;}
     
     public Semester()
     {
      AvailableCourses = new List<Course>();
     }
}

public class Course{
     public string Name {get; set;}
     public int Id {get; set;}
}

Public class SchoolYearModel{
      public List<Semester> Semesters {get; set;}
}

var semester = new Semester{ AvailableCourses = dbAcessor.GetAllCourses();}
var semester2 = new Semester{ AvailableCourses = dbAcessor.GetAllCourses();}

What am I missing, or is it just a tradeoff?

Answer 1

Some good answers here already, but it seems none of them addresses your confusion with the DRY principle, so let me do this.

Just because you don't put dbAcessor.GetAllCourses() inside the constructor of a class does not mean your code necessarily has to duplicate this statement. You can still find a location in your codebase where you can place a function like

 Semester CreateNewSemesterFromDb() 
 {
    return new Semester{ AvailableCourses = dbAcessor.GetAllCourses()};
 }

and let that be the one-and-only place where Semester objects are create from the DB. This might be a static function in another class, maybe an existing context class, or some SemesterBuilder or SemesterFactory.

Of course, one would usually design this Semester class with a constructor parameter for the available courses, to make sure the construction does not leave AvailableCourses uninitialized. But as you see from the the other answers, there are several alternatives how to do this precisely, either directly by passing the IEnumerable<Course> into the constructor, or by some function or repository which will abstract the database away and allow synchronous or asynchronous initialization. However, what makes most sense is context dependend and your question contains hardly enough information to make a sensible decision about which of these alternatives to choose.

Answer 2

What your reasoning misses is that it expects db calls to be performed synchronously within constructor. This indeed could be lengthy, unreliable etc.

Thing is though, these calls can be done asynchronously. Fields initialized in constructor can hold a "future" / "promise" so that their initialization in constructor merely starts a lengthy task asynchronously and after that, immediately returns.

This way, you can have a single invocation of db call that nevertheless doesn't block the constructor. After constructor (quickly) completes any code that needs result of this call will simply wait (outside of constructor) until its result is ready.

I use this approach every time I want to have a guaranteed single invocation of some lenghy task, not only db calls. Works like a charm.

One additional thing I would recommend is to stick with timed waits - this way you would have a way to gracefully handle cases when it takes longer than you would want to, for example, when you can't connect to db for too long.

Answer 3

The issue with the proposed approaches

The first approach assumes that the Semester object is always related to database content:

• This is very inflexible: What if you'd need a temporary semester only in memory for simulation? What if you'd need a new semester to populate it with a set of new courses?
• This couples a domain object (Semester) to a specific database access object (dbAccessor).

The second approach adresses theses two issues: you can create Semester independently of the database, and you can populate it with any database accessor able to return a list. Nevertheless it has still an important issue: the using context of Semester (probably a business logic layer or a view) would need to know about the internals of the object, since it has to populate them from the database.

Alternatives

Your first approach looks very much like a variant of the active record pattern. You could easily get rid of the main issues by allowing to construct the objects empty, and add an explicit read() function to populate them when it is needed.

But database access logic would still be tightly coupled to the business logic (it's the main drawback of this pattern). To get rid of this issue, you could inject the database object at construction, but keep it unused.

Alternatively, you may want to learn more about the repository pattern.

Answer 4

IMHO, this would be a code smell.

First of all, constructors should show the dependencies of a class. In your case, Semester needs the AvailableCourses to work correctly. Why hide it?

Also, you are hard-coding that all Semester should contain all courses in the DB. What if you want to offer different courses on different semesters? What if the courses have pre-conditions that the students must meet?

I feel that, by using DB calls, you make testing harder. First, you turn a unit test into something more like an integration test (as you need a DB to run them). Then, you force any tests that remotely interact with a Semester to talk with the DB. You might use in-memory DB or mock it, but the underlining issue remains.

Finally, I think constructors should be straightforward and cheap. For more complex initializations/construction you can always check a creational design pattern.

Hope it helps!

Answer 5

Your second example doesn't require the database to be called twice at all. The second call is blatantly unnecessary.

The most sensible approach would be to call the database once, and then assign the same result to both objects which are using that result at the same point in time.

Database calls are relatively expensive in most real-world systems - an expense that may not become apparent until the system is under proper load by multiple users.

Any application that is written in such a way as to be so extravagant with database calls, will almost certainly fail to perform in practice.

Writing the database call directly into the constructor obviously makes it impossible for objects to share the same local copy of the courses list, which is why baking the call into the constructor wouldn't normally be an acceptable practice.

Another issue here is that in real-world systems, databases reside somewhere else on a network. They are not an extension of the main memory of the programmer's machine.

To use an analogy, it's the difference between what paperwork exists on your private desk, and the archive department down the hallway which is used by dozens or hundreds of different staff a day, and even has its own internal staff, and opening hours.

This remoteness and haphazardness of access to the records, requires relatively complicated logic to handle correctly.

Because each call to the database - when done correctly - has an overhead of complications and inefficiencies, the standard practice is to make as few calls as possible.

This should be considered as naturally as that you don't walk down the hall to the records department to get a fresh copy of the course list ten times in 5 minutes - you get one copy, then use that one copy to process all items on your own desk that require that course list, (and perhaps reuse the same list for the rest of the day, if not the rest of the year).