HTTP request and HTTP client abstractions

Question

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 <string>
#include <vector>

class httpRequest 
{
    std::string method;
    std::string httpVer;
    std::string host;
    std::string fullHost;
    std::string uri;
    std::vector<std::string> 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 <string>
#include <iostream>

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 < this->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 + " ";
}

void httpRequest::addHeader(std::string httpHeader)
{
    this->headers.push_back(httpHeader + "\r\n");
}


void httpRequest::buildRequest()
{
    this->fullRequest = this->method + this->uri + this->httpVer + "\r\n";
    this->fullRequest = this->fullRequest + this->fullHost + "\r\n"; 
    this->fullRequest = this->fullRequest + this->parseHeaders() + "\r\n";
    this->fullRequest = this->fullRequest + "\r\n";
}


std::string httpRequest::getHost()
{
    return this->host;
}


std::string httpRequest::toString()
{
    return this->fullRequest;
}

int httpRequest::getRequestLength()
{
    return this->fullRequest.length();
}

The HTTP client:

httpClient.h

#ifndef httpClient_H
#define httpClient_H

#include <iostream>
#include <winsock2.h>
#include <windows.h>
#include <winsock.h>
#include <ws2tcpip.h>
#include <iphlpapi.h>
#include "httpRequest.h"

class httpClient 
{
    WSADATA wsaData;
    SOCKET connectSocket;
    std::string ip;
    std::string resp;
    struct addrinfo *result, *ptr, hints;

    void connectTo(std::string ip);
    std::string receive(httpRequest request);

    public:
        httpClient();
        void sendRequest(httpRequest request);

        std::string getResponse();      
};

#endif

httpClient.cpp

#include <iostream>
#include <winsock2.h>
#include <windows.h>
#include <winsock.h>
#include <ws2tcpip.h>
#include <iphlpapi.h>
#include "httpClient.h"
#include "httpRequest.h"
#include <string>

#pragma comment(lib, "ws2_32.lib")
#pragma GCC diagnostic ignored "-Wwrite-strings"

#define DEFAULT_PORT "80"

httpClient::httpClient()
{
    int wsResult = 0;

    wsResult = WSAStartup(MAKEWORD(2,2), &this->wsaData);
    if (wsResult != 0) {
        std::cout<<"WSAStartup failed with error: "<<wsResult<<std::endl;   
    }

    ZeroMemory( &this->hints, sizeof(this->hints) );

    this->hints.ai_family = AF_UNSPEC;
    this->hints.ai_socktype = SOCK_STREAM;
    this->hints.ai_protocol = IPPROTO_TCP;
}

void httpClient::connectTo(std::string ip)
{   
    int wsResult = 0;

    wsResult = getaddrinfo(ip.c_str(), DEFAULT_PORT, &this->hints, &result);
    if ( wsResult != 0 ) {
        std::cout<<"getaddrinfo failed with error: "<<wsResult<<std::endl;
        WSACleanup();
    }

    for(this->ptr=this->result; this->ptr != NULL ;this->ptr=this->ptr->ai_next) {
        // Create a SOCKET 
        this->connectSocket = socket(this->ptr->ai_family, this->ptr->ai_socktype, 
            this->ptr->ai_protocol);
        if (this->connectSocket == INVALID_SOCKET) {
            std::cout<<"Socket failed with error: "<<WSAGetLastError();
            WSACleanup();    
        }

        // Connect to server.
        wsResult = connect( this->connectSocket, this->ptr->ai_addr, (int)this->ptr->ai_addrlen);
        if (wsResult == SOCKET_ERROR) {
            closesocket(this->connectSocket);
            this->connectSocket = INVALID_SOCKET;
            continue;
        }
        break;
    }

    freeaddrinfo(result);

    if (this->connectSocket == INVALID_SOCKET) {
        std::cout<<"Unable to connect to server!\n";
        WSACleanup();
    }
}

std::string httpClient::receive(httpRequest request)
{    
     int iResult = 0, requestLength = 0, iSent = 0;
     requestLength = request.getRequestLength();
     char buffer[10054];

     do {
         iResult = send(this->connectSocket, request.toString().c_str(), requestLength - iSent, 0 );
         iSent = iSent + iResult;
            } while(iSent < requestLength);

            iResult = recv(this->connectSocket, buffer, 10054, 0);
                if(iResult > 0)
                    {
                        //std::cout<<buffer<<std::endl;
                    }
         else if ( iResult == 0 )
            printf("Connection closed\n");
         else
            printf("recv failed with error: %d\n", WSAGetLastError());

    closesocket(this->connectSocket);
    WSACleanup();

    std::string s = buffer;

    return s;
}

void httpClient::sendRequest(httpRequest request)
{   
    this->connectTo(request.getHost());
    this->resp = this->receive(request);
}

std::string httpClient::getResponse()
{
   return this->resp;
}

How this works is :

    //You create the request
    httpRequest r;
    //Set the host,uri and headers
    r.setHost("www.learncpp.com");
    r.setUri("/");
    r.addHeader("Connection: close");
    //Build the request
    r.buildRequest();

    //Create the client
    httpClient *cl = new httpClient()
    //Send the request
    cl->sendRequest(r);

//Get the Response
cout<<cl->getResponse()<<endl;

Now I want to know, since I am an absolute C++ beginner (though I do have experience in web programming), what I could improve in terms of OOP design and client architecture.

Answer 1

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<std::string> 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 < this->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.size(); ++i)  // Note prefer prefix increment.
    {
        parsedHeaders += this->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 httpRequest::parseHeaders()

This should be:

std::string const& httpRequest::parseHeaders() const

Getters/Setters break encapsulation. Prefer not to use them. Create the appropriate constructors.

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

This would be a good opportunity to use the PIMPL pattern.

class httpClient 
{

};

This would allow you to abstract away the windows implementation into a sub class. A future developer could then add the appropriate definition for a Linux or apple implementation at a latter stage.

Some code in the constructor failed.

    wsResult = WSAStartup(MAKEWORD(2,2), &this->wsaData);
    if (wsResult != 0) {
        std::cout<<"WSAStartup failed with error: "<<wsResult<<std::endl;   
    }

But you still allowed the object to be constructed? Now you have an object in an inconsistent state and no way to detect that the object is in a bad state. You should just throw an exception at this point. Let the caller decide if it is worth continuing by catching any exceptions.