2

I have the following StudentGroup class:

public class StudentGroup
{
    [Key]
    public int Id { get; set; }

    //the set of peers who will review the work of the StudentGroup
    [ForeignKey("PeerReviewGroupId")]
    public PeerReviewGroup PeerReviewGroup { get; set; }
    public int? PeerReviewGroupId { get; set; }

    public IEnumerable<GroupMembership> GroupMemberships { get; set; }
}

Derived from this class is PeerReviewGroup, in which there are two new properties:

public class PeerReviewGroup: StudentGroup
{
    [ForeignKey("StudentToReview")]
    public ApplicationUser StudentToReview { get; set; }
    public string StudentToReviewId { get; set; }

    [ForeignKey("StudentGroupToReview")]
    public StudentGroup StudentGroupToReview { get; set; }
    public string StudentGroupToReviewId { get; set; }

}

PeerReviewGroup could be assigned to an individual (i.e., StudentToReview) or another group (i.e., StudentrGoup). So, either of these values has to be null.

Instead, I could drive two classes:

public class PeerReviewGroupForIndividuals: StudentGroup
{
    [ForeignKey("StudentToReview")]
    public ApplicationUser StudentToReview { get; set; }
    public string StudentToReviewId { get; set; }
}

public class PeerReviewGroupForGroups: StudentGroup
{
    [ForeignKey("StudentGroupToReview")]
    public StudentGroup StudentGroupToReview { get; set; }
    public string StudentGroupToReviewId { get; set; }

}

I wonder if this is preferable or not, or which approach is the best practice. I think deriving two classes complicates things. Any suggestions?

3
  • So you have two groups of students and one group can review the other group?
    – Shai Cohen
    Commented Oct 10, 2019 at 5:48
  • @ShaiCohen yes, but the PeerReviewGroup can also review a student. It does not have to be a StudentGroup.
    – renakre
    Commented Oct 10, 2019 at 6:01
  • 1
    This is usually a tradeoff between normalization/less redundancy and simplicity. It depends on the context what is more important in the specific case.
    – Doc Brown
    Commented Oct 10, 2019 at 6:52

3 Answers 3

2

I feel you may be over-complicating the problem.

Why would the following approach not work:

public class StudentGroup
{
    [Key]
    public int Id {get; set;}

    //the students who comprise the group
    //the list can contain 1 to n users
    public List<ApplicationUser> Members
}

public class PeerReviewSession
{
    //this is the group of students who are performing the review
    [ForeignKey("GroupReviewing")]
    public StudentGroup GroupReviewing {get; set;}

    //this is the group of students who are being reviewed
    [ForeignKey("GroupReviewed")]
    public StudentGroup GroupReviewed {get; set;}

}

There may be some elements missing here, since I do not know the entire scenario. However, if your basic premise is as I suggested in the comments:

"you have two groups of students and one group can review the other group"

Then this is really all you need.

1

PeerReviewGroup could be assigned to an individual (i.e., StudentToReview) or another group (i.e., StudentGroup). So, either of these values has to be null.

Instead, I could drive two classes

At face value, your interpretation is correct. Since you never need both properties at the same time, there is no purpose to putting them in the same entity/table.

However, it seems reasonably possible that a given (existing) peer review group can be assigned to a student today, and to a student group tomorrow. If you split the entities, that means that you have to recreate your peer review group every time they switch being assigned to a student or a group.

I don't know enough about your application to make that call.

  • If a peer review group is a persistent entity that survives multiple assignments over time, then you either keep the properties together or look for a more abstracted structure (see my suggestion below)
  • If a peer review group is a discardable entity which you recreate every time a new assignment is made, then you can favor splitting the entities using inheritance.

Entity inheritance is a really powerful tool that can help with keeping things sane; but as always, overapplying inheritance leads to insanity down the line.


Alternate suggestion

Another approach to this can be to not derive the peer review group, but rather provide a base class between student and student groups. Something along the lines of:

public abstract class PeerReviewSubject
{
    public string Id { get; set; }
}

public class Student : PeerReviewSubject { }

public class StudentGroup : PeerReviewSubject { }

public class PeerReviewGroup : StudentGroup
{
    [ForeignKey("Subject")]
    public PeerReviewSubject Subject { get; set; }
    public string SubjectId { get; set; }
}

I don't like the string IDs you're using, but that's a different battle.

This achieves the same result, but it won't require you to keep extending PeerReviewGroup whenever a new peer-reviewable entity type is added to the domain. Instead, every new peer reviewable entity type simply derives from PeerReviewSubject and can therefore be assigned to a peer review group.

I made PeerReviewSubject abstract in the example. That's not required, but it is the way to go if you don't want to create barebones PeerReviewSubject objects without further derivation.

2
  • Thanks for the detailed response! Very much appreciated. The PeerReviewGroup and StudentGroup will be specific to an assignment and they will not persist afterwards. Also, if PeerReviewGroup is assigned to a StudentGroup (or Student), it will not change later (to a Student or to a different StudentGroup. Based on this information, do you think I should go for Inheritance or not?
    – renakre
    Commented Oct 10, 2019 at 7:57
  • @renakre: I personally think my alternate suggestion is a better approach to entity inheritance for your particular use case, but both approaches will get you to where you're trying to go.
    – Flater
    Commented Oct 10, 2019 at 8:36
1

To answer this question, you need to think about the lifecycle of your PeerReviewGroup:

  • is it ALWAYS known at its creation what kind of group it is ?
  • is it ABSOLUTELY excluded that the same group changes its kind of review subject once it is created ?

If the answer is yes to both questions, you could consider making two classes. But this does not necessarily mean that you should.

A more fundamental question is whether a PeerReviewGroupforIndividuals IS-A really specific PeerReviewGroup very different from the others (inheritance) or if it is just a PeerReviewGroup that just HAS-A different Subject to study (composition). After all, nothing looks closer to a peer review reviewing a group than a peer review group reviewing an individual. The same group could even do both.

The frontier between the two alternatives can be somewhat fuzzy. In case of doubt:

Prefer composition over inheritance

Your Answer

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

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