HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Having models which inherit from one base model

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromhavingonewhichinheritmodelsmodelbase

Problem

I have a model such as this below that acts as a base model. Several other models inherit from this one since they will all share common properties. Would an approach like this fit best practices or would it be better to go another route. I'm aware that composition is always considered a best practice.

public class BaseServiceResponse
{
    #region Properties

    public Guid PatientId { get; set; }
    public Guid PracticeId { get; set; }
    public ServiceStatusCodes Status { get; set; }
    public string Message { get; set; }
    public string ReturnUrl { get; set; }

    #endregion

    #region Constructors

    public BaseServiceResponse()
    {
        InitMembers();
    }

    public BaseServiceResponse(Guid patientId)
    {
        this.PatientId = patientId;
    }

    #endregion

    #region Private Methods

    private void InitMembers()
    {
        this.PatientId = Guid.Empty;
        this.PracticeId = Guid.Empty;                    
        this.Status = 0;
        this.Message = string.Empty;
        this.ReturnUrl = string.Empty;            
    }
    #endregion
}


I then have several other models which inherit from it such as this one

public class FullReportResponse : BaseServiceResponse
{
    #region Properties

    public List ReportNames;
    public string SomeOtherProp;

    #endregion

    #region Constructors

    public FullReportResponse(Guid patientId, List reportNames)
    {
        this.PatientId = patientId;
        this.ReportNames = reportNames;
    }

    #endregion
}

Solution

I'm aware that composition is always considered a best practice.

I wouldn't say that it's always the best choice. Though I guess that's not what you're saying, since you chose inheritance anyway.

In general, best practices are just guidelines to keep in mind when designing and implementing your code.

In this specific case, I think that inheritance is the right solution.

#region Properties


I don't see the need to have these kinds of #regions all over your code. Your classes aren't humongous so normal code folding will work just fine.

InitMembers();


Why is this code in a separate method? Unless you have a good reason for that, just put it directly into the constructor.

public BaseServiceResponse(Guid patientId)
{
    this.PatientId = patientId;
}


This means that the other members aren't going to be initialized. You should probably delegate from one constructor to the other using : this() to fix this.

this.PatientId = Guid.Empty;
this.PracticeId = Guid.Empty;
this.Status = 0;
this.Message = string.Empty;
this.ReturnUrl = string.Empty;


The empty GUID and 0 are the default values for Guid and an enum, respectively, so you don't need to initialize them to that. Though it might make sense to do it anyway for clarity.

Initializing the string properties to empty string makes it less clear that the value is uninitialized and could potentially hide bugs.

public List ReportNames;
public string SomeOtherProp;


These should be properties too, not public fields. And when you make ReportNames a field, consider making the setter private.

public FullReportResponse(Guid patientId, List reportNames)
{
    this.PatientId = patientId;
    this.ReportNames = reportNames;
}


Consider making a copy of reportNames here. Otherwise, it's easy to write buggy code that passes the list to FullReportResponse and then reuses it for other purposes, which will change the FullReportResponse.

Also, consider accepting IEnumerable to be more flexible.

Code Snippets

#region Properties
InitMembers();
public BaseServiceResponse(Guid patientId)
{
    this.PatientId = patientId;
}
this.PatientId = Guid.Empty;
this.PracticeId = Guid.Empty;
this.Status = 0;
this.Message = string.Empty;
this.ReturnUrl = string.Empty;
public List<string> ReportNames;
public string SomeOtherProp;

Context

StackExchange Code Review Q#78880, answer score: 7

Revisions (0)

No revisions yet.