7

Suppose I have the following class:

public class Course {
    // data
    public string Name { get; }
    public List<Student> Students {get;}
    //...

    // logic
    public int AverageGrade() {/* do something*/}
    public bool ReachedMaxStudents(){/* do something */}
}

I have some operations that interact with the database such as inserting a student to a class (should be done in the database as well.) What is the best approach to this?

First approach: developer has to know the course Id and pass it to repository.

public class CourseRepository : ICourseRepository{
    public void InsertStudent(int CourseId, StudentDto student){
        /* do something */
    }
}

Second approach: embed repository inside the domain object:

public class Course {
    private ICourseRepository _repo;

    // data
    public string Name { get; }
    public List<Student> Students {get;}
    //...

    // logic
    public int AverageGrade() {/* do something*/}
    public bool ReachedMaxStudents(){/* do something */}
    public void AddStudent(Student student){
        StudentDto s = /* map Student to Studentdto */
        _repo.Insert(s);
    }
}

What are the pros and cons to each approach and which one is preferred?

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
Husain
  • 309
  • 2
  • 7
  • 6
    Any repository worth its salt would not require domain objects to possess knowledge of how to save themselves to the database. Ergo, I prefer the first approach. Let your repository worry about how to get the objects saved to the database; that's the whole point of having a repository layer. – Robert Harvey Oct 05 '17 at 20:18
  • @RobertHarvey Thank you. But here where the problem arise. Suppose that adding a student involves some logic (for example, the student should be an active student and should maintain a certain GPA and have the correct major, etc.) Should that be done in the domain object? how would the business object interact with the repository in this case? – Husain Oct 05 '17 at 20:24
  • 2
    You can have validation logic in your domain object; either add an `IsValid()` method or write a constructor that takes parameters and automatically constructs a valid domain object. – Robert Harvey Oct 05 '17 at 20:47
  • Regarding validating objects— often you will need both the constructor to set the object into a valid initial state as well as a way to validate that the object is still in a valid state. Many frameworks provide concepts and code to handle this. You should research how your framework does so. If you’re not using a framework or yours doesn’t support this, then you’ll need to write your own validation methods— but do your best not to duplicate the validation code within the class. – RibaldEddie Oct 05 '17 at 21:37
  • @RobertHarvey: did you read the code? He actually asks if the domain object should have knowledge about **the repository**, not about the database. I made a little change in the text to correct this possible misconception. – Doc Brown Oct 06 '17 at 05:22
  • 1
    Releated: https://softwareengineering.stackexchange.com/questions/352156/should-a-rich-domain-model-have-repositories-injeted-in-some-situations – Doc Brown Oct 06 '17 at 06:25
  • 1
    See also: https://stackoverflow.com/questions/20035810/should-domain-objects-have-dependencies-injected-into-them – Doc Brown Oct 06 '17 at 06:30
  • @DocBrown: Ah, yes, I see what you're saying. Given the mere use of an `IRepository` interface (which already provides a substantial amount of decoupling), I'm OK with injecting an implementation into the domain object and letting the domain object sort out all of the business logic specifically pertaining to that domain object, if that is a convenient way to do it. – Robert Harvey Oct 06 '17 at 15:02

1 Answers1

9

No, domain objects should not have any idea of the existence of a repository, or even a repository interface. If you feel the need to do this, that means there is an underlying issue with your design (see below).

The root of your problem is that you have not modeled all of your entities properly.

A course does not have students it has registrations.

Not only is your missing entity causing your problems where you feel objects need to know how to persist themselves (they don't); it will cause you problems on edge situations... For example, what happens if your student registers for a course, then cancels their registration, but then changes their mind again and reregisters for a course? Your current model cannot handle this situation if it has to do any sort of historical analysis (perhaps there is a cancellation fee that needs to be levied).

You need at least six different classes here: Registration, Student, Course, RegistrationRepository, StudentRepository, and CourseRepository.

Make sure your repositories only try to save its direct entity... do not have the CourseRepository try to save all of a courses registrations.

Your code would like something like this...

//This would be a method in the UI, maybe called when a button is clicked... ect
void UI_RegisterStudentForCourse(Student studentToReg, int courseId)
{
    //Register student for course
    Course course = courseRepo.GetCourse(courseId);
    course.Registrations = regRepo.GetStudents(courseId);

    Registration reg = new Registration();
    reg.Course = course;
    reg.Student = studentToReg;
    reg.Date = DateTime.Now;

    if (course.CanRegister(reg)) // Check if the course is full, ect
       regRepo.Save(reg);
    else
       //Tell the user this registration could not be completed
}

The problem with the second approach you listed is, it makes your code very brittle. What if you need to run a simulation where things should not be saved? What if you want to run unit-tests against your registration logic? It would require extensive mocking. Additionally, when your code violates the Single Responsibility principal (mixing saving with domain logic in this case), it makes things difficult to maintain and changes to the code are much more risky.

TheCatWhisperer
  • 5,231
  • 1
  • 22
  • 41
  • This is all correct, nevertheless it IMHO fails to answer the main question: should domain objects hold references to repositories (repository interfaces)? For example, your code example above makes use of two repos. Does it belong into some method of a domain object? (My recommendation would be "no", but your answer leaves that out). – Doc Brown Oct 06 '17 at 06:38
  • 1
    @DocBrown Thanks for pointing that out, answer edited – TheCatWhisperer Oct 06 '17 at 14:06
  • 1
    Right, having the correct entity model is more important than the details of how code is structured. No amount of good code will fix an incorrect model. This is true in general of course. There might not be any silver bullets, and there certainly are no vampires, but there is a lot of bad thinking going on. –  Oct 06 '17 at 14:30
  • @nocomprende I would be hesitant to call it "bad thinking" (but maybe that is just systematics)... I was *better* at modeling domains *before* I started coding professionally. It took me about six years to relearn to do it. We have a lot of bad concepts and frameworks that throw people off. Labeling a table "xref" or "join" makes people not think about the entity. Entity framework thoroughly confuses people as to what an entity is... ect... – TheCatWhisperer Oct 06 '17 at 14:35
  • OK, well, my phrase is a bit provoking, but you are correct that often the main enemy is terminology. I have seen many arguments over the definition of words, or where people did not even realize that the definitions differed. I think only one person actually understands Entity Framework (Julie / Julia Lerman) and everyone else is most likely lost. They have had to reinvent it at least once, and it continues to be a great solution which is way too powerful for most problems. Sometimes I think we would be better off just taking one expert as gospel and perfecting whatever technique they offer. –  Oct 06 '17 at 14:52
  • Sorry, but I still fail to see how your answer explains **why** a domain class should not hold a repo reference, or what this actually has to do with the improved model you suggested. I see you suggest to put the orchestration logic into some (UI, really?) method, but no explanation why this orchestration could not also be part of a domain object's method. IMHO a good place could be also a controller class inside the domain layer, but currently I don't have a compelling argument against what the OP suggested. – Doc Brown Oct 06 '17 at 15:25
  • @user251748 : So how the heck do you find out where to get these _true_ understandings from, of _any_ of these kinds of concepts, anyways? – The_Sympathizer Feb 19 '19 at 08:53
  • 1
    Hi, does `course.CanRegister(reg)` need to access the database? If it needs to access the database, how can that be done? Thanks. – jflaga Sep 26 '19 at 12:00
  • So, I am still not getting the point. How can I store some data which is produced inside a domain model? You are saying that domain model should know nothing about repositories. But then there is no way to persist data. What is used instead of a repository then? – manymanymore Apr 08 '21 at 13:33