Which option is better Programming Practice

Question

I am writing a process that creates an extension of the Hammock REstRequest object, and wanted to know which implementation (which is very similar) would be considered the better programming practice. Here is the object definition, which has 2 constructors

public class RequestObject : RestRequest
{    
    public RequestObject()
    {
        this.Encoding = System.Text.Encoding.UTF8;
        System.Net.ServicePointManager.Expect100Continue = false;
    }

    public RequestObject(WebMethod webMethod, byte[] ContentArray)
        : this()
    {
        if (webMethod == Hammock.Web.WebMethod.Put
            || webMethod == Hammock.Web.WebMethod.Post)
        {
            //encode payload string to a utf8 byte array
            this.AddPostContent(ContentArray);
            this.AddHeader("Content-Type", "application/json");
        }
    }
}

For the construction of the object, there are 2 ways I can do it, instantiate it inline, as follows:

string payload = serializer.Serialize(user);

RequestObject request
    = new RequestObject(WebMethod.Put, BuildAPI.GetBytes(payload))
      {
          Credentials = new BasicAuthCredentials
          {
              Username = ConfigurationManager.AppSettings["UserName"],
              Password = ConfigurationManager.AppSettings["Password"]
          },
          UserAgent = ConfigurationManager.AppSettings["UserAgent"],
      };

Or call a member of a "Build Class" to construct the object:

public static RequestObject BuildRequest(WebMethod method, byte[] byteArray)
{
    RequestObject request = new RequestObject(WebMethod.Put, byteArray)
    {
        Credentials = new BasicAuthCredentials
        {
            Username = ConfigurationManager.AppSettings["UserName"],
            Password = ConfigurationManager.AppSettings["Password"]
        },
        UserAgent = ConfigurationManager.AppSettings["UserAgent"],
        Method = method
    };

    return request;
}

and call it as follows:

RequestObject request = BuildAPI.BuildRequest(WebMethod.Put, payload);

I am just wondering which of the two options is considered better programming practices. I am reading into SOLID and now I am nervous about multiple "functions" being in the same class, but don't want to over think things.

Answer 1

Starting way before the SOLID principles, you're code is very incoherent. You have some static BuildAPI - so either decide on sticking to it or don't use it. Because it's static, this decision should be based on whether you suspect that in the future these constructions will need to be polymorphic, or not.

Second note: do you really need two constructors? Seems like only one is used.

Third note: You initialize some of the properties in constructor, and some with the property initializer - why not just stick to one paradigm (POD object, or a class)?

Finally, as for the SOLID principles - your RequestObject clearly violates the Single Responsibility Principle which is evident from the fact, that you pass an enum to the constructor. So this class represents in fact few classes which are fit together. Consider making RequestObject abstract, and implement a separate sub-class for each request type.

Something like this:

abstract class RequestObject
{
    //properties and methods common to all requests

    protected RequestObject(...)
    {
        //initialize common properties
    }
}

class GetRequest : RequestObject
{
    public GetRequest()
    {
        this.Encoding = System.Text.Encoding.UTF8;
        System.Net.ServicePointManager.Expect100Continue = false;
    }

    //... other specific things for GET
}

class PutRequest : RequestObject
{
    public PutRequest()
    {
        this.AddPostContent(ContentArray);
        this.AddHeader("Content-Type", "application/json");
    }

    //... specific things for PUT
}

... you get the picture. Probably you could make also another level between RequestObject and Post, Put as they also probably share some behaviour.