4
\$\begingroup\$

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<string> ReportNames;
    public string SomeOtherProp;

    #endregion

    #region Constructors

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

    #endregion
}
\$\endgroup\$
7
\$\begingroup\$

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<string> 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<string> 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<string> to be more flexible.

\$\endgroup\$
4
\$\begingroup\$

I'd add on to svick's suggestions to also ask the question, is instantiating a BaseServiceResponse valid, or would it only be valid to instantiate a derivation of BaseServiceResponse.

If not valid, then consider making BaseServiceResponse an abstract class with protected constructors.

I mean, its not that you don't trust yourself, its more to make sure you don't accidentally do something you didn't intend to. Although it is unnecessary, a base class is usually not intended to be instantiated.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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