Reading a date from the start of a text webpage from github to determine if the program is fully updated

Question

#define WIN32_LEAN_AND_MEAN
#include <stdio.h>
#include <string.h>
#include <windows.h>
#include <wininet.h>

const char thisVersionDate[] = "2024-01-05";


bool isMostRecentVersion(HINTERNET* hInternet, HINTERNET* hConnection, HINTERNET* hData) {
    char currentVersionDate[sizeof(thisVersionDate)] = {};

    *hInternet = InternetOpenA(
        "wininet_test",
        INTERNET_OPEN_TYPE_PRECONFIG,
        nullptr,
        nullptr,
        0
    );
    if (*hInternet == nullptr) {
        printf("error when using InternetOpenA: %d\n", GetLastError());
        return false;
    }

    *hConnection = InternetConnectA(
        *hInternet,
        "raw.githubusercontent.com",
        INTERNET_DEFAULT_HTTPS_PORT,
        nullptr,
        nullptr,
        INTERNET_SERVICE_HTTP,
        0,
        0
    );
    if (*hConnection == nullptr) {
        printf("error when using InternetConnectA: %d\n", GetLastError());
        return false;
    }

    *hData = HttpOpenRequestA(
        *hConnection,
        nullptr,
        "/speedrun-program/amnesia_load_screen_tool/main/amnesia_settings.txt",
        nullptr,
        nullptr,
        nullptr,
        INTERNET_FLAG_SECURE,
        0
    );
    if (*hData == nullptr) {
        printf("error when using HttpOpenRequestA: %d\n", GetLastError());
        return false;
    }

    if (!HttpSendRequestA(*hData, nullptr, 0, nullptr, 0)) {
        printf("error when using HttpSendRequestA: %d\n", GetLastError());
        return false;
    }

    // windows API documentation says to call this in a loop
    DWORD bytesReadThisLoop = 0;
    DWORD totalBytesRead = 0;
    while (
        totalBytesRead < sizeof(currentVersionDate) - 1
        && InternetReadFile(*hData, &currentVersionDate[totalBytesRead], sizeof(currentVersionDate) - 1 - totalBytesRead, &bytesReadThisLoop)
        && bytesReadThisLoop != 0) {
        totalBytesRead += bytesReadThisLoop;
    }

    if (totalBytesRead < sizeof(currentVersionDate) - 1) {
        printf("couldn't determine the date of the most recent version using InternetReadFile: %d\n", GetLastError());
        return false;
    }

    bool isMostRecentVersion = (strncmp(thisVersionDate, currentVersionDate, sizeof(currentVersionDate) - 1) == 0);

    if (strncmp(thisVersionDate, currentVersionDate, sizeof(currentVersionDate) - 1) != 0) {
        printf("a newer version from %s is available\nthis version's date: %s\n", currentVersionDate, thisVersionDate);
        return false;
    }

    return true;
}


int main() {
    HINTERNET hInternet = nullptr;
    HINTERNET hConnection = nullptr;
    HINTERNET hData = nullptr;

    // resources acquired:
    // hInternet (1)
    // hConnection (2)
    // hData (3)
    if (isMostRecentVersion(&hInternet, &hConnection, &hData)) {
        printf("this is the most recent version\n");
    }

    if (hData != nullptr) {
        InternetCloseHandle(hData);          // hData released (3)
    }
    if (hConnection != nullptr) {
        InternetCloseHandle(hConnection);    // hConnection released (2)
    }
    if (hInternet != nullptr) {
        InternetCloseHandle(hInternet);      // hInternet released (1)
    }

    return 0;
}

I'm writing a program in C++ for, and I'd like it to be able to check if it's fully updated. My idea is to have it read some text from a webpage on github representing the date of the most recent version. It can then compare the date on github to its own date.

Answer 1

Overall, the code is structured and easy to follow. Some of the remarks below can probably be ignored in the context of this small program, they only become more important when dealing with a larger codebase, they are still something worth being aware of.

Use Of Pointer Parameters

Some of your functions use pointer arguments. These are then dereferenced without checking the pointer values against null first. At the very least, use assert(). Better perhaps would be to use references, which implicitly can not be null. Even better would be to return values from the functions (e.g. isMostRecentVersion()), maybe using tuples to return multiple values.

Use Of WinAPI *A Functions

These legacy functions (from the DOS-derived Windows versions) convert their parameters internally before calling the according *W functions (which are Unicode-capable). I'd generally prefer the non-legacy versions.

sizeof With Redundant Parentheses

There are two ways to use sizeof, as sizeof (type) or as sizeof object. The latter doesn't require any parentheses.

Not Using Exceptions

There is a lot of code which logs an error with some context info and then returns false to signal failure. This is bad for multiple reasons:

• The return value slot is occupied. This means you can't use it to return the actual results of the function to the caller and that you have to use pointers/references (see above) to convey those results. Note that you could use std::optional<> to either return a falsy value or actual results.
• The context info is only logged to stdout. That means that the calling function only sees success/failure but doesn't receive any info why something failed. If you wanted to use this function in a UI, you'd first have to intercept stdout in order to present the error to the user. If you wanted to handle specific errors (e.g. using one resource by default but with a fallback when that resource is not available), you couldn't do so easily.
• You end up with repeated error-checking code in the calling function as well, which is tedious to read and write.
• You can't distinguish the case that you don't have the latest version from the case that determining the version failed. Both are signalled using return false.

Use Of Manual Resource Management

Just as raw pointers with manual memory management are a bad idea, use of raw HINTERNET handles are. Using a smart handle to encapsulate this handle and to reliably release the resource it represent is a better approach, also known as RAII.

Returning Unused Resources From isMostRecentVersion()

The function allocates resources and assigns them to output values. However, these output values are never really used by the calling code, they are only deallocated. Keep that inside the function. Also, use RAII for that (see above).

Nitpicks

• I guess this code has a very restricted scope. Otherwise, is most recent version begets the question "what?". I'd normally expect some kind of parameter telling the function what or where to check the version.
• The location where to check a version is provided by some hardcoded inline constants, which are like "Magic Numbers". The currentVersionDate constant does this much better.
• "a newer version from %s is available", the "from" doesn't make sense in that message.