WinInet C++ Wrapper

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

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_PRECONFI

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.

StackExchange Code Review Q#69801, answer score: 4