6

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.

  • 5
    I'd say it's o.k., especially if you foresee ReturnRequest adding functionality not in BaseRequest. – user949300 Jan 27 '17 at 0:21
  • 3
    Seems fine to me. – paparazzo Jan 27 '17 at 0:28
11

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.

  • The names I used in my example aren't representative of my actual code, I just named them that way for this question. – maccettura Jan 27 '17 at 15:34
  • I have updated my question a bit if you have time to look again. – maccettura Jan 27 '17 at 15:41
  • 1
    The prop names are much better. Still not happy with BaseRequest. I'd prefer a name that tells me why it should include CashierId, Amount, and PosId but not ZipCode. Base only tells me that something is inheriting from this, which isn't really any of this classes business to know. Request only tells me someone is requesting something. What's missing is a clear idea of why CashierId, Amount, and PosId come together. Preferably, a word that would mean something to a domain expert. – candied_orange Jan 27 '17 at 20:48
  • 1
    I consider this important to your question because ReturnRequest is stealing a good name from BaseRequest. To me the problem with the design is that it's asking for more good names than we can think of. Which is a bad excuse to limit your number of classes. It's a good excuse to take a walk and think. – candied_orange Jan 27 '17 at 20:51
  • LoginException is a technically valid example, but it's a special fringe case. Marker interfaces (the same principle applies to marker subtypes) are usually considered as a less-than-good fix for something that should've been handled a different way (e.g. attributes or properties). Exceptions slightly avoid that judgment because a "marker subtype" is sometimes necessary due to catch being explicitly designed to work based on the exception's type rather than content. – Flater Jun 11 at 9:55
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).

  • This is exactly what I was thinking. Just because two classes may have similar fields doesn't mean those fields are the same. They might be called the same and even represent the same thing (such as price), but they are fundamentally different. – Erdrik Ironrose Jan 27 '17 at 9:48
0

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.

  • I like the statement "It's not a common abstraction, until you have code that expects and works on 'BaseRequest' directly." As long as no code refers to the base class, the inheritance acts more like a mixin than inheritance and should be fine. Once you start using the base type directly and maybe downcasting, then you have a problem. – MPavlak Jan 27 '17 at 14:44

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.