patterncsharpMinor
Having models which inherit from one base model
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.
I then have several other models which inherit from it such as this one
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.
I don't see the need to have these kinds of
Why is this code in a separate method? Unless you have a good reason for that, just put it directly into the constructor.
This means that the other members aren't going to be initialized. You should probably delegate from one constructor to the other using
The empty GUID and 0 are the default values for
Initializing the string properties to empty string makes it less clear that the value is uninitialized and could potentially hide bugs.
These should be properties too, not public fields. And when you make
Consider making a copy of
Also, consider accepting
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 PropertiesI 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 PropertiesInitMembers();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.