WinInet C++ Wrapper

Question

Because I don't like complex sequences of code for simple functions, I wrote a wrapper for http/https access in A++. Are there any bad programming practices I am using? Is there any functionality I should add? One thing I have not figured out yet is how to determine content-length if it's not specified in the header. How do I do that?

http.h

#include <Windows.h>
#include <WinInet.h>

typedef struct {
    LPSTR url;
    HINTERNET request;
    HINTERNET connection;
} HTTP_REQUEST;

DWORD HTTPInitRequest(HTTP_REQUEST* id, LPCSTR url);
DWORD HTTPSetHeader(HTTP_REQUEST* id, LPCSTR name, LPCSTR data);
DWORD HTTPSendRequest(HTTP_REQUEST* id, LPDWORD responseCode, LPDWORD contentLength, LPCSTR verb = "GET", LPCSTR postData = NULL);
DWORD HTTPReadRequest(HTTP_REQUEST* id, LPSTR responseBuffer, DWORD bufferLength, LPDWORD bytesRead);
//Errors

#define HTTP_ERR_SUCCESS 0x0 //Everything completed successfully

#define HTTP_ERR_COULD_NOT_CONNECT 0x1 //The computer is either not connected to the internet or an invalid domain was specified

#define HTTP_ERR_NO_SESSION 0x2 //The function failed to create a session

#define HTTP_ERR_COULD_NOT_SEND_REQUEST 0x3

#define HTTP_ERR_QUERY_FAILED 0x4

#define HTTP_ERR_INVALID_CONNECTION 0x5

#define HTTP_ERR_COULD_NOT_READ_FILE 0x6

#define HTTP_ERR_HEADER_NOT_SET 0x7

http.cpp

#pragma comment(lib, "wininet.lib")
#include "http.h"

BOOL isHttps(LPCSTR url);

BOOL isHttp(LPCSTR url);

BOOL isHttpProtocol(LPCSTR url);

BOOL strEqual(LPCSTR a, LPCSTR b);

LPSTR getDomain(LPCSTR url);

LPSTR getPath(LPCSTR url);

LPCSTR postMIME = "Content-Type: application/x-www-form-urlencoded";

HINTERNET hSession = NULL;

LPCSTR userAgent = "Mozilla/5.0 (Windows NT 6.3;)";

DWORD HTTPInitRequest(HTTP_REQUEST* id, LPCSTR url) {

    id->url = new CHAR[lstrlen(url) + 1];

    lstrcpy(id->url, url);

    BOOL https = isHttps(id->url);

    if (!isHttpProtocol(id->url)) return FALSE;

    if (!hSession) hSession = InternetOpen(userAgent, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, NULL);

    if (!hSession) {
        return HTTP_ERR_NO_SESSION;
    }

    id->connection = InternetConnect(hSession, getDomain(id->url), https ? INTERNET_DEFAULT_HTTPS_PORT : INTERNET_DEFAULT_HTTP_PORT, NULL, NULL, INTERNET_SERVICE_HTTP, NULL, NULL);

    if (id->connection) {
        return HTTP_ERR_COULD_NOT_CONNECT;
    }

    return HTTP_ERR_SUCCESS;
}

DWORD HTTPSetHeader(HTTP_REQUEST* id, LPCSTR name, LPCSTR data) {
    LPSTR header = new CHAR[lstrlen(name) + lstrlen(": ") + lstrlen(data) + 1];

    lstrcpy(header, name);
    lstrcat(header, ": ");
    lstrcat(header, data);
    header[lstrlen(header)] = NULL;

    if (!HttpAddRequestHeaders(id->request, header, -1, HTTP_ADDREQ_FLAG_ADD | HTTP_ADDREQ_FLAG_REPLACE)) {
        return HTTP_ERR_HEADER_NOT_SET;
    }

    return HTTP_ERR_SUCCESS;
}

DWORD HTTPSendRequest(HTTP_REQUEST* id, LPDWORD responseCode, LPDWORD contentLength, LPCSTR verb, LPCSTR postData) {

    BOOL post = strEqual(verb, "POST");

    BOOL https = isHttps(id->url);

    id->request = HttpOpenRequest(id->connection, verb, getPath(id->url), "HTTP/1.1", NULL, NULL, https ? INTERNET_FLAG_SECURE : NULL, NULL);

    if (!HttpSendRequest(id->request, post ? postMIME : NULL, post ? lstrlen(postMIME) : NULL, (LPVOID) (post ? postData : NULL), post ? lstrlen(postData) : NULL)) {
        return HTTP_ERR_COULD_NOT_SEND_REQUEST;
    }

    DWORD buffSize = sizeof(DWORD);

    if (!HttpQueryInfo(id->request, HTTP_QUERY_STATUS_CODE | HTTP_QUERY_FLAG_NUMBER, responseCode, &buffSize, NULL)) {
        return HTTP_ERR_QUERY_FAILED;
    }

    if (!HttpQueryInfo(id->request, HTTP_QUERY_CONTENT_LENGTH | HTTP_QUERY_FLAG_NUMBER, contentLength, &buffSize, NULL)) {
        return HTTP_ERR_QUERY_FAILED;
    }

    return HTTP_ERR_SUCCESS;
}

DWORD HTTPReadRequest(HTTP_REQUEST* id, LPSTR responseBuffer, DWORD bufferLength, LPDWORD bytesRead) {
    if (!id->request || !id->connection) return HTTP_ERR_INVALID_CONNECTION;

    if (!InternetReadFile(id->request, responseBuffer, bufferLength, bytesRead)) {
        InternetCloseHandle(id->request);
        InternetCloseHandle(id->connection);
        return HTTP_ERR_COULD_NOT_READ_FILE;
    }


    responseBuffer[*bytesRead] = NULL;

    InternetCloseHandle(id->request);
    InternetCloseHandle(id->connection);

    return HTTP_ERR_SUCCESS;
}

LPSTR getDomain(LPCSTR url) {

    BOOL https = isHttps(url);

    LPCSTR sig = https ? "https://" : "http://";

    LPVOID lv = (LPVOID) url;

    LPSTR index = (LPSTR) lv;

    for (int i = 0; i < lstrlen(sig); i++) {
        if (url[i] == sig[i])  {
            index++;
            continue;
        }
        else return NULL;
    }

    DWORD domainLength = 0;

    int i = 0;
    while (index[i++] != '/') domainLength++;

    LPSTR result = new CHAR[domainLength+1];

    memcpy(result, index, domainLength);

    result[domainLength] = NULL;

    return result;
}

LPSTR getPath(LPCSTR url) {

    BOOL https = isHttps(url);

    LPCSTR sig = https ? "https://" : "http://";

    LPVOID lv = (LPVOID)url;

    LPSTR index = (LPSTR)lv;

    for (int i = 0; i < lstrlen(sig); i++) {
        if (url[i] == sig[i])  {
            index++;
            continue;
        }
        else return NULL;
    }

    DWORD domainLength = 0;

    int i = 0;
    while (index[i++] != '/') domainLength++;
    i = 0;

    index += domainLength;

    DWORD pathLength = 0;

    while (index[i++] != NULL) pathLength++;

    LPSTR result = new CHAR[pathLength];

    lstrcpy(result, index);

    return result;
}

BOOL isHttpProtocol(LPCSTR url) {
    return isHttps(url) ? TRUE : isHttp(url);
}

BOOL isHttps(LPCSTR url) {
    LPSTR https = "https://";

    for (int i = 0; i < lstrlen(https); i++) if (url[i] != https[i]) return FALSE;

    return TRUE;
}

BOOL isHttp(LPCSTR url) {
    LPSTR http = "http://";

    for (int i = 0; i < lstrlen(http); i++) if (url[i] != http[i]) return FALSE;

    return TRUE;
}

BOOL strEqual(LPCSTR a, LPCSTR b) {
    return !lstrcmp(a, b);
}

This may or may not work with MinGW or Cygwin, but it's intended for Visual Studio.

** The license for the code may be found here

** Don't use wide characters

Answer 1

Your code might be compiled as C++, but it is C by heart. If you are willing to spend some time to make it more like C++ (perhaps as a learning experience) here are some changes you could apply:

Prefer classes over structs and loose functions:

The main unit of work in C++ is the class, so your code should instead look something like this:

class HttpRequest {
public:
    // Constructors/destructor...

    DWORD InitRequest(LPCSTR url);
    DWORD SetHeader(LPCSTR name, LPCSTR data);
    DWORD SendRequest(LPDWORD responseCode, LPDWORD contentLength, LPCSTR verb = "GET", LPCSTR postData = nullptr);
    DWORD ReadRequest(LPSTR responseBuffer, DWORD bufferLength, LPDWORD bytesRead);

private:
    LPSTR url;
    HINTERNET request;
    HINTERNET connection;
};

Avoid macros:

Macros are pretty clumsy and don't respect scope. C++ offers much better ways of declaring constants, such as const, enum and constexpr.

I would use an enum for your error codes. If you have a C++11 compiler, enum class is even better for scoped constants:

enum class HttpCode {
    Success             = 0x0,
    CouldNotConnect     = 0x1,
    NoSession           = 0x2,
    CouldNotSendRequest = 0x3,
    QueryFailed         = 0x4,
    InvalidConnection   = 0x5,
    CouldNotReadFile    = 0x6,
    HeaderNotSet        = 0x7
};

Now they are used like this: HttpCode result = HttpCode::Success;. You even get some extra type safety and self documentation in there, vs the opaque DWORD.

About the error codes:

However, some of your HTTP_ERRs would be better implemented as exceptions. Error codes are a somewhat crude way of handling errors that are so frequently used in C for the lack of a better option. Exceptions can greatly simplify that, but don't overuse them. Exceptions are for exceptional things.

Prefer the standard string:

The LPSTR Windows type and friends look quite ugly in the code. They are just typedefs for char pointers. std::string is so much better and safer. Give is a try some time.

Prefer nullptr:

If your compiler is C++11 compliant (recent versions of VS are), then use nullptr. It is a big improvement over the oldschool NULL macro.

Use smart pointers:

If you switch to std::string, you no longer need to manually allocate memory for strings. For most other cases, however, you should be using smart pointers to manage the lifetime of dynamic memory. Manual memory management is a dated practice that has shown over and over again to result in memory leaks, memory corruption bugs and dangling pointers. Smart pointers have been around long enough and have proven their value. I strongly recommend their use.

Answer 2

In http.h, I'd change the last five defines to look like

#define HTTP_ERR_COULD_NOT_SEND_REQUEST 0x4
#define HTTP_ERR_QUERY_FAILED 0x8
#define HTTP_ERR_INVALID_CONNECTION 0x10
#define HTTP_ERR_COULD_NOT_READ_FILE 0x20
#define HTTP_ERR_HEADER_NOT_SET 0x40

That way you don't have to choose between errors if you don't want to do so. You have the option of returning any or all the errors at once.

I also removed the extra line breaks.

There's no reason to separate these defines.

They go together, so you can just put them next to each other.

Putting them on separate lines is like breaking a paragraph into one paragraph for each line.

Doesn't this seem like overkill?

Also, it's not clear what goes together and what does not.

You separate by the same amount between things in a group as you do for things not in a group.

In http.cpp, you shouldn't have to include all those prototypes. Just define the functions in the order in which they're used unless you have two functions that call each other. Obviously in that case, at least one will need to be prototyped. However, I don't see that in any of your functions.

In HTTPInitRequest, I have the same problem with spacing. Everything is separate, so I can't see what goes with what. I might reorganize it as follows:

DWORD HTTPInitRequest(HTTP_REQUEST* id, LPCSTR url) {
    id->url = new CHAR[lstrlen(url) + 1];
    lstrcpy(id->url, url);

    if ( ! isHttpProtocol(id->url) ) {
        return FALSE;
    }

    if (!hSession) {
        hSession = InternetOpen(userAgent, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, NULL);
    }

    if (!hSession) {
        return HTTP_ERR_NO_SESSION;
    }

    INTERNET_PORT port = isHttps(id->url) ? INTERNET_DEFAULT_HTTPS_PORT : INTERNET_DEFAULT_HTTP_PORT;
    id->connection = InternetConnect(hSession, getDomain(id->url), port, NULL, NULL, INTERNET_SERVICE_HTTP, NULL, NULL);

    if (id->connection) {
        return HTTP_ERR_COULD_NOT_CONNECT;
    }

    return HTTP_ERR_SUCCESS;
}

Should that be if ( ! id->connection ) { instead? It seems unusual for a successful connection to evaluate as boolean false.

First, I moved your https variable closer to the single statement that used it. Then I looked at it more and skipped the intermediate variable entirely in favor of a port variable. This way there isn't a ternary operator embedded in a function call. You could put back the https variable if you wanted. I had some trouble finding where https was used.

I also moved the memory allocation and copy into id->url together. That makes it more obvious that they are associated.

I have much the same kind of comments for HTTPSendRequest as for HTTPInitRequest.

In getDomain, you have

BOOL https = isHttps(url);

LPCSTR sig = https ? "https://" : "http://";

Why not just say

LPCSTR protocol = isHttps(url) ? "https://" : "http://";

Also, why sig and not protocol? I've been looking at it for a while now and I still don't know what a sig is.

LPVOID lv = (LPVOID) url;

LPSTR index = (LPSTR) lv;

What's this cast out of LPSTR and then back to LPSTR get you? If it gets you something, this would be a great place for a comment.

for (int i = 0; i < lstrlen(sig); i++) {

You should try to avoid calling the same function multiple times with the same exact input.

for ( int i = 0, n = lstrlen(sig); i < n; i++ ) {

Later, you have

DWORD domainLength = 0;

int i = 0;
while (index[i++] != '/') domainLength++;

LPSTR result = new CHAR[domainLength+1];

What happens if there is no trailing / in the URL string? You should explicitly check that you don't go past the end of the string.

int indexLength = lstrlen(index);
DWORD domainLength = 0;
while ( ( index[domainLength++] != '/' ) && ( domainLength <= indexLength ) ) ;

LPSTR result = new CHAR[domainLength--];

Similar comments for getPath plus

DWORD pathLength = 0;

while (index[i++] != NULL) pathLength++;

LPSTR result = new CHAR[pathLength];

Why not just say

LPSTR result = new CHAR[lstrlen(index) + 1];

In general, you are using a lot of C notions plus the non-standard LPSTR variables. Presumably this helps with interacting with the Windows functions, but it forces you do more work than you should.

Answer 3

I'm taking a wild guess that you were a C programmer and are now doing a C++ project.

1. Change:

   typedef struct {
       LPSTR url;
       HINTERNET request;
       HINTERNET connection;
   } HTTP_REQUEST;
   

   To:

   struct HTTP_REQUEST
   {
       LPSTR url;
       HINTERNET request;
       HINTERNET connection;
   };
   

   In C++, you do not have to typedef the struct in order to skip typing:

   struct HTTP_REQUEST httpRequest;
   

2. Instead of using LPSTR url; in your HTTP_REQUEST struct, use either an std::string or if you are building an MFC application, use a CString. Let a pre-built container manage your memory instead of managing the memory yourself. These containers can also be used as buffers.

3. Get rid of the #define flags and replace them with an actual type, such as const unsigned. This may sound counter-intuitive, but it gives you type-safety and it won't cost any additional memory because the compiler will simply optimize them out (replace the variables with literals (this gives you the same behavior as macros)).

4. Prefer references instead of pointers. Only use pointers when a NULL type is acceptable.

   For example:

   DWORD HTTPInitRequest(HTTP_REQUEST* id, LPCSTR url);
   

   Could become:

   DWORD HTTPInitRequest(HTTP_REQUEST& id, const std::string& url);
   

   If you are willing to use exceptions, it can become even cleaner:

   HTTP_REQUEST HTTPInitRequest(const std::string& url);