4

I have a method that accepts a simple class object and builds a URL used in an API call. I would like this method to be able handle/accept different class types that are similar yet have different properties.

public class ClientData
{
   public string Name {get; set;}
   public string Email {get; set;}
   ...
}

public class PaymentData
{
   public decimal PaymentAmount {get; set;}
   public string Description {get; set;}
   ...
}

Below are two example methods. As you can see they are very similar. Would it better to implement these as different methods accepting different parameters or can one method be written that can deal with parameter object differences?

public string BuildApiCall(ClientData clientDataObject)
{
  StringBuilder sb = new StringBuilder();
  sb.Append("http://mytestapi.com/");
  sb.append("name=" + clientDataObject.Name);
  sb.append("email=" + clientDataObject.Email);
  return sb.ToString();
}

public string BuildApiCall(PaymentData paymentDataObject)
{
  StringBuilder sb = new StringBuilder();
  sb.Append("http://mytestapi.com/");
  sb.append("payment=" + paymentDataObject.PaymentAmount );
  sb.append("description=" + paymentDataObject.Description );
  return sb.ToString();
}
vgru
  • 49,838
  • 16
  • 120
  • 201
webworm
  • 10,587
  • 33
  • 120
  • 217
  • 2
    You could let both implement the same interface and accept that in the method. That interface should have at least `Name` and `Email` as properties. (However, your code seems to be wrong since `PaymentData` does not have `Name` and `Email` even if you use it). – Tim Schmelter Jul 29 '14 at 15:00
  • 3
    @TimSchmelter: And perhaps a `ToApiString()` method. Each class should be responsible for knowing how to generate its API call. – Cᴏʀʏ Jul 29 '14 at 15:01
  • I think what Tim said is the best approach. I thought the same thing, myself. – Brian Jul 29 '14 at 15:02
  • There are lots of ways you could do this and it really depends on the scope and what you feel is the appropriate level of abstraction. It's a difficult question to answer without more context. But I do like @Cory's idea. Define an interface with a `ToApiString()` method and have both classes use it. The `ToApiString()` part would return just the part of the string that is specific to that class. – Matt Burland Jul 29 '14 at 15:02
  • 1
    It does not look like the URL your are making is correct. You have a domain and a couple of query string arguments. Do both of these call the same web page or is the web page different? Also, you need the proper delimiters between the query string values. – Gridly Jul 29 '14 at 15:05
  • another way you can use is to define a generic method BuildApiCall(T item) where T:class,ICall... where ICall is an Interface that contains Email and Name properties, hope this will help you – Monah Jul 29 '14 at 15:07
  • Sorry for the confusion. The API call is not real. I created it as part of the example. – webworm Jul 29 '14 at 15:07
  • If you can change the sources of `ClientData` and `PaymentData` then user @dodexahedron has answered your question.. – Sriram Sakthivel Jul 29 '14 at 15:08
  • @Hadi - Could you explain a bit further as an answer? I would like to understand more about that technique. – webworm Jul 29 '14 at 15:08
  • @HadiHassan: I fail to see what a generic method adds versus just defining an interface. Especially since you also define an interface and restrict your generic method to just that interface! – Matt Burland Jul 29 '14 at 15:09
  • if you will put the properties and the method inside the interface, then you should define the method in each class, while if you made the method as generic, you need to define it once. if you have let us say 20 different classes with same properties, Name, Email, then you need to define 20 times the method since you will put it in the interface, while you will define it once out side the classes. – Monah Jul 29 '14 at 15:12
  • @HadiHassan: I think you are completely missing the point of generics. You can define an interface with just properties rather than a method if you want, but you really aren't solving anything with generics the way you are using them. – Matt Burland Jul 29 '14 at 15:14
  • after your comment, i checked the question again and you are right two classes with [Different properties and not same properties as i thought], so my propose will not solve the problem, it will work to make an interface with the method and all the classes will implement this interface. – Monah Jul 29 '14 at 15:16
  • @Cory Each class should most certainly not know how to generate it's own API call. What if the API changes, what if you want to use it with multiple API's? http://en.wikipedia.org/wiki/Single_responsibility_principle – Mikkel Løkke Jul 30 '14 at 07:06

2 Answers2

7

Deciding which approach to take

Your problem is, essentially, to create a custom serializer for your classes, based on the provided API (presumably fixed).

To separate concerns as much as possible, this functionality is commonly implemented separately from your entity classes, leaving them (if anyhow possible) as POCOs, or dumb DTOs which are serialization independent, as much as possible. So, just like you would use a XmlSerializer or a DataContractSerializer to serialize a class into XML, or Protobuf.NET to serialize it into protocol buffers, arguably the most general approach would be to create your own serializer.

Of course, as with all other problems you encounter in daily programming, you need to weigh potential benefits and decide how much effort you want to invest into refactoring. If you have a small number of cases, nobody will get hurt by a couple of copy/pasted hard-coded methods, similar to what you're doing now. Also, if this is just a tiny "pet project", then you might decide that you don't want to waste time on potential issues you might encounter while trying to refactor into a more general solution (which you might never need again).

Your goal is to have to write as little as possible

However, if you do choose to invest some time in writing a serializer, then you will quickly note that most serialization frameworks try to rely on convention for serialization as much as possible. In other words, if your class is:

public class ClientData
{
    public string Name { get; set; }
    public string Email { get; set; }
}

Then a XmlSerializer will, without any configuration at all, produce the following XML:

<ClientData>
    <Name>...</Name>
    <Email>...</Email>
</ClientData>

It would also be really cool to have a class which would simply spit out ?name=...&email=... for that object, with absolutely no additional work on your side. If that works, than you have a class which will not only remove duplication from existing code, but seriously save time for all future extensions to the API.

So, if you are writing your classes based on the API, then it might make sense to name properties exactly like API members whenever possible (and use convention-based serialization), but still keep it open enough to be able to handle a couple of edge cases separately.

Example code

public class ClientData
{
    public string Name {get; set;}
    public string Email {get; set;}
}

// customer really insisted that the property is
// named `PaymentAmount` as opposed to simply `Amount`,
// so we'll add a custom attribute here
public class PaymentData
{
    [MyApiName("payment")]
    public decimal PaymentAmount {get; set;}
    public string Description {get; set;}
}

The MyApiName attribute is really simple, just accepts a single string argument:

public class MyApiNameAttribute : Attribute
{
    private readonly string _name;
    public string Name
    { get { return _name; } }

    public MyApiNameAttribute(string name)
    { _name = name; }
}

With that in place, we can now use a bit of reflection to render the query:

public static string Serialize(object obj)
{
    var sb = new StringBuilder();

    foreach (var p in obj.GetType().GetProperties())
    {
        // default key name is the lowercase property name
        var key = p.Name.ToLowerInvariant();

        // we need to UrlEncode all values passed to an url
        var value = Uri.EscapeDataString(p.GetValue(obj, null).ToString());

        // if custom attribute is specified, use that value instead
        var attr = p
            .GetCustomAttributes(typeof(MyApiNameAttribute), false)
            .FirstOrDefault() as MyApiNameAttribute;

        if (attr != null)
            key = attr.Name;

        sb.AppendFormat(
            System.Globalization.CultureInfo.InvariantCulture,
            "{0}={1}&",
            key, value);
    }

    // trim trailing ampersand
    if (sb.Length > 0 && sb[sb.Length - 1] == '&')
        sb.Length--;

    return sb.ToString();
}

Usage:

var payment = new PaymentData()
{
    Description = "some stuff",
    PaymentAmount = 50.0m 
};

// this will produce "payment=50.0&description=some%20stuff"            
var query = MyApiSerializer.Serialize(payment)

Performance

As noted in the comments, the power of reflection does incur a performance penalty. In most cases, this should not be of much concern. In this case, if you compare the cost of building the query string (probably in the range of 10s of microseconds) with the cost of executing the HTTP request, you'll see that it's pretty much negligible.

If, however, you decide that you want to optimize, you can easily do it at the end, after profiling, by changing that single method which does all the work by caching property information or even compiling delegates. That's good about separation of concerns; duplicated code is hard to optimize.

Community
  • 1
  • 1
vgru
  • 49,838
  • 16
  • 120
  • 201
  • This seems like a very very large hammer for what the OP is asking. KISS? – dodexahedron Jul 29 '14 at 15:18
  • I like this solution as being the most flexible. It's arguable whether it might be overly flexible (I would at least still restrict the method to taking an interface or a base class, even if it's only a marker interface). – Matt Burland Jul 29 '14 at 15:23
  • @dodexahedron: but that's what OP asked, if I got it right: "is it possible to create a single method to serialize two different objects". Using attributes to provide metadata for serialization is pretty common in all serialization frameworks, so I believe it's the right approach. *And* this is the only code that needs to be written (well, copy/pasted from here actually). It took me 5 minutes to write it, and it can handle 10 more similar classes in the same manner, without code duplication, so I don't really feel like this "hammer" weighs that much. – vgru Jul 29 '14 at 15:23
  • He did. But sometimes the most generic solution is not necessarily the best, for runtime efficiency reasons (unnecessary reflection where a very small amount of code duplication with an interface solves the problem anyway). Credit where credit is due, though, your answer is great and seemingly achieves exactly what OP asked for, so it gets an upvote from me for sure. – dodexahedron Jul 29 '14 at 15:29
  • @dodexahedron: why did you delete your answer? It got several upvotes, and there's nothing wrong with having several approaches. – vgru Jul 29 '14 at 18:26
  • Because @MattBurland's was the same path as mine, but even better. Just saving some clutter. – dodexahedron Jul 29 '14 at 18:37
  • This is not "unnecessary reflection", and there is no evidence to support the claim that this is "less efficient" however you want to measure that. Having said this, this is by far the most elegant and maintainable solution for three reasons. First, if the data structure changes you only have to change it in one place. Second, if the API changes you only have to change it in one place. Lastly it complies with the Single Responsibility Principle. – Mikkel Løkke Jul 30 '14 at 07:05
4

Define an interface:

public interface IWhatsit
{
     string ToApiString();
}

Now have your data objects implement it. ToApiString should return the query string part for this particular object:

public class ClientData : IWhatsit
{
   public string Name {get; set;}
   public string Email {get; set;}
   ...

   public string ToApiString()
   {
       // Do whatever you need here - use a string builder if you want
       return string.Format("Name={0}&Email={1}",Name,Email);
   }
}

And now you can have a single method for making the API call:

public string BuildApiCall(IWhatsit thing)
{
  StringBuilder sb = new StringBuilder();
  sb.Append("http://mytestapi.com/");
  sb.append(thing.ToApiString());
  return sb.ToString();
}

Note: You could use a property instead of a method in the interface if you so wished.

An alternative approach would be to use an abstract base class and inherit from that. Then you could do something like this:

public abstract class BaseData
{
    protected abstract string ToApiString();

    public string BuildApiCall()
    {
        StringBuilder sb = new StringBuilder();
        sb.Append("http://mytestapi.com/");
        sb.append(ToApiString());
        return sb.ToString();
    }
}

And then each class would look something like this:

public class ClientData : BaseData
{
   public string Name {get; set;}
   public string Email {get; set;}
   ...

   protected override string ToApiString()
   {
       // Do whatever you need here - use a string builder if you want
       return string.Format("Name={0}&Email={1}",Name,Email);
   }
}

Which lets you put BuildApiCall inside the class itself and have a base implementation. Of course, if you do need BuildApiCall to be outside of these classes, then you could do that. It would just take a BaseData and you'd have to make ToApiString public rather than protected.

Matt Burland
  • 44,552
  • 18
  • 99
  • 171
  • 1
    This is actually much better than what I wrote. ++ – dodexahedron Jul 29 '14 at 15:33
  • Why create an interface? Couldn't I just create the `BuildApiCall` in each class? Not sure how the interface helps. – webworm Jul 29 '14 at 16:12
  • You create the interface so that you can have one method that can handle either type of object. Now, if it's the case that your `BuildApiCall` is actually *inside* these classes (your question isn't clear), then it's a different question altogether. In that case, I'd have them both inherit from a (possibly abstract) base class with a `virtual` `ToApiString()` method that you can override in your derived classes. If you create the `BuildApiCall` in each class, then you are back where you started! – Matt Burland Jul 29 '14 at 16:51
  • The problem with this is you're essentially violating the SRP, as each class will have multiple responsibilities. One being knowledge of the domain, the other being knowledge of the domain. You also have duplicated code, because the API call is essentially identical for everything the implements the interface. – Mikkel Løkke Jul 30 '14 at 06:56
  • @MikkelLøkke: I believe in pragmatism over dogmatism, but I'm illustrating one way to solve the problem. You can, of course, change the `ToApiString()` to something more agnostic if you want. For example, you could just return a dictionary of property names and values so that the class doesn't have to worry about the format of the query string. Whether or not that's worth the effort is a "big picture" question that the OP would have to answer for themselves. As for duplicated code, I don't know what you are talking about there. The `BuildApiCall` only needs to exist in one place. – Matt Burland Jul 30 '14 at 12:43
  • @MattBurland As do I, however not to the extreme, and in this particular case I don't really see how sharing a common interface is more pragmatic than using reflection to build the string. The duplicated code is in the ToApiString, because once you've had to change that for the 10th time, the pragmatic approach is to just use reflection, and instead of having that in every class, just move it to the one that understands the APi already. :) – Mikkel Løkke Jul 30 '14 at 13:38
  • @MikkelLøkke: The "duplicated" code is one line? And it's a `string.Format`? And it's not duplicated because it'll be different for each implementation? I agree, as I said in my previous comment, that it does fix the query string format, but if that's a problem you can just return something agnostic like a dictionary of property names and values (and probably change the name of the function) and let the caller build whatever it needs from that. That said, I do like Groo's answer and even said so. – Matt Burland Jul 30 '14 at 13:43