C# OOP – Is It Poor Practice to Implement Abstract Class Without New Functionality?

cdryobject-orientedsolid

I am writing a wrapper for a REST API and I have run into something I have never had to ask myself before.

This API is for E-Commerce transactions, it has SALE and RETURN endpoints (and some other endpoints that aren't critical to this discussion).

The SALE and RETURN requests are nearly identical, with the exception that the SALE request has an additional property.

public class ReturnRequest
{
    [JsonProperty("cashier_id")]
    public string CashierId { get; set; }

    [JsonProperty("amount")]
    public decimal Amount { get; set; }

    [JsonProperty("pos_id")]
    public CustomClass PosId { get; set; } 
}

public class SaleRequest
{
    [JsonProperty("cashier_id")]
    public string CashierId { get; set; }

    [JsonProperty("amount")]
    public decimal Amount { get; set; }

    [JsonProperty("pos_id")]
    public CustomClass PosId { get; set; } 

    [JsonProperty("zip_code")]
    public string ZipCode { get; set; }
}

Originally I just had the SALE request POCO inherit the RETURN request POCO:

public class ReturnRequest
{
    [JsonProperty("cashier_id")]
    public string CashierId { get; set; }

    [JsonProperty("amount")]
    public decimal Amount { get; set; }

    [JsonProperty("pos_id")]
    public CustomClass PosId { get; set; }  
}

public class SaleRequest : ReturnRequest
{
    [JsonProperty("zip_code")]
    public string ZipCode { get; set; }
}

But to me that didn't seem very intuitive or clear (SALE and RETURN are different things, why would one inherit another?)

I then decided to put the common properties into a base abstract class:

public abstract class BaseRequest
{
    [JsonProperty("cashier_id")]
    public string CashierId { get; set; }

    [JsonProperty("amount")]
    public decimal Amount { get; set; }

    [JsonProperty("pos_id")]
    public CustomClass PosId { get; set; }  
}

public class ReturnRequest : BaseRequest
{

}

public class SaleRequest : BaseRequest
{
    [JsonProperty("zip_code")]
    public string ZipCode { get; set; }
}

But then that leaves me with a class that is essentially the same thing as the BaseRequest.

At first I justified this because I wanted the classes to be straightforward and verbose. It creates a clear difference between the classes and I can modify the SALE or RETURN without worrying about the other (or even the base classs).

However now I am wondering if I am thinking through this wrong, is having essentially a blank class a bad idea?

EDIT: I changed the property names and types up a bit to more closely represent some of the types of properties.

Best Answer

My favorite example of inheriting and adding nothing new is this:

public class LoginException : System.ApplicationException{}

Why? Because I've said all I need to say with that wonderfully descriptive class name. Sure if I want to add a dynamic message I'll need to add a constructor. But if I don't, why should I?

Your example lacks this clarity because the semantic issues are hidden with names like Prop1. I love replacing inheritance with composition and delegation but in this case, when I do that, I end up looking at this:

SaleRequest sr = new SaleRequest(new ReturnRequest());

Now I gotta ask, does that make semantic sense? I can't request a sale without requesting a return?

Sure, mechanically it makes sense because the 'Props' are 'correct' but those are just datatypes. Would it still make sense if they had meaningful names?

Your attempt to fix this problem looks like this:

SaleRequest sr = new SaleRequest(new BaseRequest());

Which again is semantically anemic. Base? That doesn't tell me what belongs in this class and what doesn't belong in it. That's not a good name.

If BaseRequest had a name that made it clear that Prop1 and Prop2 (whatever those are) belong in it and BaseRequest had a name for something that is clearly part of, or all of, the other two requests then I'd be fine with this.

As it stands, I don't know what is supposed to be going on when I look at this code. That's not a good thing.

I have no problem with empty classes. But please, give them good names. It's really the only thing they have to offer.

Related Topic