3
\$\begingroup\$

I'm learning C# as a web developer rather than a C# developer learning web development. I've gotten the hang of creating models and controllers (or so I think) for a web project, and I've read some tutorials on general C# and I'm just putting it all together. I'd like to know how I can best refactor this code.

Let's say I have the following model

// Models for Lite Requests
public class RequestLite
{
    public string action { get; set; }
    public string action2 { get; set; }
    public string action3 { get; set; }
    public ParametersLite parameters { get; set; }
}

public class RequestHeavy
{
    public string action { get; set; }
    public string action2 { get; set; }
    public string action3 { get; set; }
    public ParametersHeavy parameters { get; set; }
}

// Models for Heavy Requests
public class ParametersLite 
{
    public List<string> parameter { get; set; }
}

public class ParametersHeavy
{
    public List<string> parameter { get; set; }
    public string membership { get; set; }
    public string featured { get; set; }
}

RequestLite and RequestHeavy are pretty much the same except for the fact that RequestHeavy has some additional parameters. The remote service can only accept one specific kind of data request set per query. If I were to send a RequestHeavy model with membership and featured set as null to a Query that only accepts RequestLite's model, it'd break.

Question: what's the best way to refactor these models? It's redundant to have ParametersLite and a ParametersHeavy if the only difference is two additional parameters. How can I just have one type of Object where I can have it send those parameters only when needed?

Here's the rest of the code

public class HomeController : Controller
{
    public string endpoint = "http://generic.api.com:12345/query";

    public string ConnectToEndpoint(string data)
    {
        var httpWebRequest = (HttpWebRequest)WebRequest.Create(endpoint);
        httpWebRequest.ContentType = "text/json";
        httpWebRequest.Method = "POST";
        var result = "";

        using (var streamWriter = new StreamWriter(httpWebRequest.GetRequestStream()))
        {
            streamWriter.Write(data);
            streamWriter.Flush();
            streamWriter.Close();

            var httpResponse = (HttpWebResponse)httpWebRequest.GetResponse();
            using (var streamReader = new StreamReader(httpResponse.GetResponseStream()))
            {
                result = streamReader.ReadToEnd();
            }
        }

        return result;
    }

    [HttpGet]
    public ActionResult Index()
    {   
        var ReqLite = new RequestLite {
            action = "GetUsers",
            action2 = "xyz",
            action3 = "zyx",
            parameters = new ParametersLite {
                parameter = new List<string> {
                    "female", "20+", "single"
                }
            }
        };

        var ReqHeavy = new RequestHeavy() {
            action = "GetSilverFeaturedUsers",
            action2 = "xyz",
            action3 = "zyx",
            parameters = new ParametersHeavy {
                parameter = new List<string> {
                    "female", "20+", "single"
                },
                membership = "silver",
                featured = "yes"
            },
        };

        // assign both responses to a view model object            

        return View();
    }

}

I've been doing all from one controller as this was the best way to learn what I needed to. Now I want to clean it up a bit

Questions: The ConnectToEndpoint method along with the endpoint string, for the purpose of just making a web request and returning a response, where should I place this so all other controllers can use it? Should I make it a reference instead, rather than say putting it on a base controller? What should I change to make it even more generic so that it can be used to connect to other endpoints that take an object or two? The ReqLite and ReqHeavy objects, since both requests share the same values, action2, action3 and parameter, what's a better way to create something more generic and reusable with those values already populated?

The ideal approach would be to have many reusable methods and classes outside of the project from an mvc perspective but easily accessible to the project and also portability of these reusable methods and classes to other solutions as well.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you please revise your title to specify the purpose of the code itself? \$\endgroup\$ – Jamal Oct 16 '14 at 23:57
3
\$\begingroup\$

Your first question can be answered with Inheritence. ParametersHeavy should inherit from ParametersLite.

From the above link:

The new class—the derived class—then gains all the non-private data and behavior of the base class in addition to any other data or behaviors it defines for itself. The new class then has two effective types: the type of the new class and the type of the class it inherits.

Then any change made to ParametersLite will "cascade" into ParametersHeavy. This means if you ever change it, you only have to make the change once instead of twice. This also simplifies the Heavy class a little bit.

public class ParametersHeavy : ParametersLite
{
    public string membership { get; set; }
    public string featured { get; set; }
}

You can do the same for RequestHeavy and RequestLite, but you will need to Override the parameters property.

You should also do yourself and the devs you work with a favor and study up on the C# Naming Guide. Properties and Methods should be PascalCased, variables should be camelCased. Also be careful with your bracket placement. It's usual to see brackets on the new line at the same indentation level. They're all over the place in your second snippet.

\$\endgroup\$
  • \$\begingroup\$ great answer, thank you. if you could answer the second question(s) that'd be great \$\endgroup\$ – archytect Oct 17 '14 at 0:46
3
\$\begingroup\$

First it should be noted that standard naming convention in C# is to use PascalCase for public class members (fields, properties, methods) and camelCase for local variables and method parameters. It's useful to stick to standard naming conventions because it will make it easier for other C# developers to find their way around in your code.


Depending on how the objects are used rather than building two inheritance hierarchies (one for request and one for parameters) you could make the request objects generic. So something along these lines:

public class Request<T>
{
    public string Action { get; set; }
    public string Action2 { get; set; }
    public string Action3 { get; set; }
    public T Parameter { get; set; }
}

public class ParametersLite 
{
    public List<string> Parameters { get; set; }
}

public class ParametersHeavy : LiteParameters
{
    public string Membership { get; set; }
    public string Featured { get; set; }
}

Then you can have requests like:

var heavyRequest = new Request<ParametersHeavy>();
var liteRequest = new Request<ParametersLite>();

or even:

// in this case the Parameter property will be of type int
var intRequest = new Request<int>();

You can restrict the type of T with generic type constraints.


The ConnectToEndpoint method should be encapsulated in it's own class which has an interface. This can be injected into the controller which will make unit testing easier. Something along these lines:

public interface IApiConnection
{
    string Query(string data);
}


public class WebApi : IApiConnection
{
    private readonly Uri _ApiUri;

    public WebApi(Uri apiUri)
    {
        _ApiUri = apiUri;
    }

    public string Query(string data)
    {
       // ConnectToEndpoint implementation goes here
    }
}

Then you can make your controller take an IApiConnection object as parameter to query the endpoint. This will allow you to re-use it across multiple projects.

In addition to that you probably don't want to return the raw string from the query but the deserialized Json object instead. A popular library is Json.NET but .NET has one baked in as well.

\$\endgroup\$

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.