Confusion over using generics in MVC project

Question

Consider the following:

Remote service accepts SecondAction=Search and SecondAction=Update. For SecondAction=Search sending a Query parameter is sufficient enough. For SecondAction=Update, I need to send back a set of additional parameters named AdditionalQuery1 and AdditionalQuery2.

The ApiKey and Action properties are always the same. The SecondAction is different. I want the model to be as reusable as possible and use less code as possible.

I want to be able to reuse the the majority of the Request and only have it to SpecialData instead of Data to send those AdditionalQueries for when I have to do a SecondAction=Update.

public class Request<T>
{
    public string ApiKey { get; set; }
    public Details<T> Details { get; set; }
}

public class Details<T> {
    public string Action { get; set; }
    public string SecondAction { get; set; }
    public MoreDetails<T> MoreDetails { get; set; }
}

public class MoreDetails<T> { 
    public T Data { get; set; }
}

public class Data {
    public string Query { get; set; }
}

public class SpecialData : Data {
    public string AdditionalQuery1 { get; set; }
    public string AdditionalQuery2 { get; set; }
}

I've confused myself to the point I'm creating it like

 var Obj = new Request<Details<MoreDetails<SpecialData>>> {
      ApiKey = "12345",
      Details = new Details<MoreDetails<Data>>() {
           Action = "Search"
      }

doesn't look right. It doesn't seem right that I have to make all the others a generic type as well.

Answer 1

First of all, I appreciate the fact that you want to keep your code DRY as many developers are not concerned with this. Also, I like the idea of having the generic Data type. This makes your code flexible so that in the future new, more derived types of Data can be created without affected significant portions of unrelated code. That being said, there are some things I would consider.

Style Notes:

• Use consistent syntax conventions

On some lines, you have an opening curly on a new line, on others, it is on the same line as the function block / signature. Either way is fine, just make sure you're consistent.

• For generic types, give them more descriptive names than T to improve readability

Presumably, your T is a Data type (more on this later), so name your generic type parameter: TData, instead of T. This gives a more clear understanding of what is expected.

Programming Notes:

As far as your use of Generics goes, I would use something more like this (and I'll explain why):

public class Request<TData> where TData : Data {

    public Request() {
        this.RequestDetail = new Details<TData>();
    } // end default constructor

    public string ApiKey { get; set; }
    public Details<TData> RequestDetail { get; set; }

} // end class Request{TData}

public class Details<TData> where TData : Data {

    public Details() {
        this.DetailData = new MoreDetails<TData>();
    } // end default constructor

    public string Action { get; set; }
    public string SecondAction { get; set; }
    public MoreDetails<TData> DetailData { get; set; }

} // end class Details{TData}

public class MoreDetails<TData> where TData : Data { 

    public TData RequestData { get; set; }

} // end class MoreDetails{TData}

public class Data {

    public string Query { get; set; }

} // end base class Data

public class SpecialData : Data {

    public string AdditionalQuery1 { get; set; }
    public string AdditionalQuery2 { get; set; }

} // end derived class SpecialData

Then, in your implementation:

Request<Data> newRequest = new Request<Data>();
newRequest.ApiKey = "12345";
newRequest.RequestDetail.Action = "Search";
newRequest.RequestDetail.DetailData.RequestData = new Data(); //You could also use SpecialData

---

What I did:

1. Use Generic Type Constraints

Using Generic type constraints is a great way to both enhance the readability and maintainability of your code because you'll have some basic compile-time checking. It also tells future developers what your expectations are.

See here for more on this subject: http://msdn.microsoft.com/en-us/library/d5x73970.aspx

2. Give more specific names to properties and fields

Your naming of properties being the same as the class names is confusing. I renamed some of the properties to make the code easier to understand. In addition to this, I would think of more descriptive names for your classes than Detail and MoreDetail as these don't really help understand the code or how it should be used. Without knowing more about what your final aim is, I can't really provide more useful names, but you should consider being more explicit.

3. Instantiate defaults for complex types in constructors

You are passing generic type arguments for your initial request, which doesn't seem like what you want. All of your generics ultimately are interested in what TData will be, not the property chain all the way down. The generic type argument only specifies the type of, not the actual value of the property (which is why I have an additional assignment in my implementation code) that actually creates a new instance of the Data class.

By instantiating each property within the constructor, the object creation is simpler in your implementation. Additionally, your implementation should not have to be concerned with the finer details of how the class and it's property chains are initialized. The fact that your implementation code has to go so many levels deep in your properties may be a code smell. Also, I am unclear on the usefulness of the MoreDetail class.

4. Simplify object creation

The object initializer syntax you're using is helpful sometimes, but when you are creating a complex object, it's best to KISS to improve the readability of the code (remember, this isn't JavaScript ;) ).

Let me know if I need to add any clarification on these points.

Answer 2

This is reaction to comments:
firda: Why do you have all those small classes connected through properties instead of using class hierarchy? I would create SearchRequest and UpdateRequest both derived from Request, while making SecondAction virtual get or protected set. Any problem with such solution?
archytect: That seems logical, but it's not easy to grasp. Could you provide some code to better visualize it?

// most basic request
public abstract class Request {
    public string ApiKey { get; set; }
    public abstract string Action { get; }
}
// some common request where the action is the same
public abstract class MyRequest: Request {
    public override string Action {
        get { return "TheAction"; }
    }
    public abstract string SecondAction { get; }
}
// search request with query
public class SearchRequest: MyRequest {
    public override string SecondAction {
        get { return "Search"; }
    }
    public string Query { get; set; }
}
// update request with two params
public class UpdateRequest: MyRequest {
    public override string SecondAction {
        get { return "Update"; }
    }
    public string AdditionalQuery1 { get; set; }
    public string AdditionalQuery2 { get; set; }
}