HTTP request and HTTP client abstractions

I am trying to make an OOP abstraction for a very simple HTTP client. I have also created a simple abstraction for a (GET only for now) HTTP request.

I am using winsock2 sockets for Windows and examples from the internet.

This is the request class:

httpRequest.h

#ifndef httpRequest_H
#define httpRequest_H

#include 
#include 

class httpRequest 
{
    std::string method;
    std::string httpVer;
    std::string host;
    std::string fullHost;
    std::string uri;
    std::vector headers;
    std::string parsedHeaders;
    std::string fullPath;
    std::string fullRequest;

    void initialize(std::string methodName, std::string hostName, std::string uriValue);
    std::string parseHeaders();

    public:
        httpRequest();
        void setMethod(std::string methodName);
        void addHeader(std::string httpHeader);
        void setHost(std::string hostName);
        void setUri(std::string uriValue);
        void buildRequest();

        int getRequestLength();
        std::string getHost();

        std::string toString(); 
};

#endif

httpRequest.cpp

```
#include "httpRequest.h"
#include
#include

httpRequest::httpRequest()
{
this->initialize("GET", "127.0.0.1", "/");
//this->fullRequest = "You need to add host URI and/or some headers";
}

void httpRequest::initialize(std::string methodName, std::string hostName, std::string uriValue)
{
this->method = methodName;
this->httpVer = "HTTP/1.1";
this->host = hostName;
this->uri = " " + uriValue + " ";
}

std::string httpRequest::parseHeaders()
{
for(int i = 0; i headers.size(); i++)
{
this->parsedHeaders += this->headers.at(i);
}

return this->parsedHeaders;
}

void httpRequest::setMethod(std::string methodName)
{
this->method = methodName;
}

void httpRequest::setHost(std::string hostName)
{
this->fullHost = "HOST: " + hostName;
this->host = hostName;
}

void httpRequest::setUri(std::string uriValue)
{
this->uri = " " + uriValue

Code Review

A standard naming convention (if there can be said to be one). Is that user defined types start with an initial capitol letter. Other identifiers that name objects (or functions) start with an initial lower case letter. This lets you find user defined types very quickly in the mess that is the C++ syntax.

You seem to have all the variables (apart from user/password and port).

Though some seem to be derived from others.

A URI looks like this:

scheme:[//[user:password@]host[:port]][/]path[?query][#fragment]

    std::string method;
    std::string httpVer;
    std::string host;
    std::string fullHost;
    std::string uri;
    std::vector headers;
    std::string parsedHeaders;
    std::string fullPath;
    std::string fullRequest;

Not sure I like the interface to create an http request.

httpRequest();
        void setMethod(std::string methodName);
        void addHeader(std::string httpHeader);
        void setHost(std::string hostName);
        void setUri(std::string uriValue);

Then I have to call build on the request once I have created added all the parameters!

void buildRequest();

Thats not a good interface for C++. Once the constructor is finished the object should be in a usable state. I would have a helper struct that has all the objects you need that can be filled out manually. Then a constructor that takes that helper object or a full URI.

struct HTTPRequestParamaeters
   {
       std::string   method;
       std::string   host;
       std::string   path;
       std::string   query;
       std::string   fragment;
       int           port;         // note 0 defaults to 80
       std::string   user;
       std::string   password;
   };
   
   class HttpRequest
   {
       public:
           HttpRequest(HTTPRequestParamaeters const& parameters);  // from parts if you want.
           HttpRequest(std::string const& method, std::string const& uri);                    // experts can use a fully formed URI
   };

Now you can use the classic list initialization syntax;

HttpRequest  request(HTTPRequestParamaeters{"GET", "www.cplusplus.com", "reference/string/to_string"});

   // Or a full URI
   HttpRequest  request("GET", "http://www.cplusplus.com/reference/string/to_string/");

Sure I don't mind a toString() function. But the standard seems to have gone to using to_string(). Also this is really just a convenience function. All it should do is return the internal URL.

std::string toString();

So there is no need to return a copy. Return a const reference to the object.

std::string const& to_string() const;

The request length?

int getRequestLength();

Does this include the headers. If not why not just let them call to_string() and get the length of the string.

It is considered bad practice to use this->.

this->initialize("GET", "127.0.0.1", "/");

If you must do this its because you have badly named members that are being shadowed by parameters or locals. Having shadowed variables is a really bad idea as eventually there will be a mistake. Turn on the warning that tell you about shadowed variables. Then you can safely use members names explicitly (it also looks neater).

You don't need an initialize() function anymore. C++11 introduced constructor chaining.

httpRequest::httpRequest()
{
    this->initialize("GET", "127.0.0.1", "/");
    //this->fullRequest = "You need to add host URI and/or some headers";   
}

This allow you to write a constructor in terms of another constructor. This then allows you to correctly use the initializer list in the constructor and prevent members being initialized then immediately being overwritten.

httpRequest::httpRequest()
    : httpRequest("GET", "127.0.0.1", "/")
{}
httpRequest::httpRequest(std::string const& method, std::string const& host, std::string const& path)
    method(method)
    httpVer("1.1")
    host(host)
    fullPath(path)
{}

The method at() is a validated access to a container that throws on failure. If you already know that you are not going to access out of range then you should use operator[]().

for(int i = 0; i headers.size(); i++)
    {
        this->parsedHeaders += this->headers.at(i);
    }

Since you are using the size() member to make sure you stay in range then there is no need to call at(). This can be re-written as:

for(int i = 0; i headers[i];
    }

But even better would be to use the range based for loop:

for(auto const& loop: headers)
    {
        parsedHeaders += loop;
    }

If you are returning an internal member variable. Prefer to return a const reference to the object. This will prevent a copy being created. Mark it const so that the person using it can not modify the object but then they can decide if they need a copy or just need to use it as a reference (ie to get the length). No need to make a copy of all the headers just to work out the length.

```
std::string ht

StackExchange Code Review Q#115284, answer score: 14