Is it poor practice to have a class implement an abstract class without adding any new fields/members/functionality?

Question

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.

Answer 1

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.

Answer 2

Yes, it's completely reasonable to have "empty" subclasses of a base class. A different but in some ways analogous situation is wrapping a type to enforce a type distinction even when representationally there is no difference. For example:

struct Seconds {
    public Seconds(int v) { Value = v; }
    public int Value { get; }
}

struct Meters {
    public Meters(int v) { Value = v; }
    public int Value { get; }
}

There may be no reason to think that these will ever be anything other than ints but it is still valuable to wrap them so they don't get mixed up.

That said, for your particular case, it's not clear that BaseRequest corresponds to a meaningful notion. If you add a LAYAWAY option, is BaseRequest still going to be a meaningful superclass? If the answer is "no", then I'd probably just treat SaleRequest and ReturnRequest as completely separate classes. (Maybe there is some interface that does capture general features of these, but it may not be BaseRequest.) You could use BaseRequest purely internally as an implementation detail if it adds enough convenience, but that is probably not worth it in this case, and it's not a great idea conceptually (it makes your code less obvious).

Answer 3

In this case being two different endpoints I would not share the returned model, let each have their own. Even if this means, in first appearance, some 'duplicate' code. They may be the same now, but they will/might change for different reasons.

It's not a common abstraction, until you have code that expects and works on 'BaseRequest' directly. Consider their similarity coincidental.