Proj-Ascension/Client

Don't put DDP-specific stuff into Library.cpp

Closed this issue · 4 comments

You shouldn't put digital distribution platform (DDP) specific stuff into Library.cpp. Instead, define an API for digital distribution platform providers (e.g. findGames(), getExecutable(), etc) and put each one into a separate file (possibly in a Source/Platforms folder).

Otherwise, stuff will soon get very messy and unmaintainable.

This could also make it possible to choose supported DDPs at compilation time (or even make them plugin later).

I can help out, but I'm hesitant make a pull request before hearing your opinion. ;-)

elken commented

As noted here, the client has no need for anything a platform offers other than its existence on the system & where games are located, so an API would be a waste.

And as mentioned in various places, we're already looking into plugins.

Another thing to add, we will be implementing various features into plugins when a plugin API is created, including the game finding code. We don't want to waste resources creating it when we will be replacing it in the future.

As noted here, the client has no need for anything a platform offers other than its
existence on the system & where games are located, so an API would be a waste.

Then this (internal!) API just consists of two functions, getRoot() and findGames(). Maybe API is a misleading term for this - I meant that you should put the distribution-platform-specific code into seperate files.

Another thing to add, we will be implementing various features into plugins when a plugin API is created, including the game finding code. We don't want to waste resources creating it when we will be replacing it in the future.

I wasn't referring to implementing a fully featured plugin API for this - but seperating code for the different platforms from start could make it easier if you ever decide to implement them as plugins. It's just a simple copy&paste operation into separate files. findOriginGames() and findSteamGames() are even seperate functions now, but IMHO it's better to put them into dedicated files. It already becomes hard to read now, even though Uplay, GOG and others are still missing.

Anyway, just my 2 cents.

PS: Here's an example:

// Origin.cpp
bool originGetRoot(void)
{
    QDir originRoot(qgetenv("APPDATA").append("/Origin"));
    if (originRoot.exists())
    {
          return originRoot;
    }
    return NULL;
}
void originFindGames(void)
{
    /* Don't know about this function signature, could also
       take "QDir root" as argument instead  */
    QDir root = originGetRoot();
    if (root != NULL) {
        //insert code here
    }
}

// Steam.cpp
bool steamGetRoot(void)
{
    bool steamFound = false;
    QDir steamRoot;
    steamRoot.setPath("");
    #if defined(_WIN32) || defined(_WIN64)
        QSettings settings("HKEY_CURRENT_USER\\Software\\Valve\\Steam",
                           QSettings::NativeFormat);
        QString steamPath = settings.value("SteamPath").toString();
        steamRoot = QDir(steamPath);
        steamFound = steamRoot != QDir::currentPath();
    #elif defined(__APPLE__)
        // TODO: however OS X handles steam
    #elif defined(__linux__)
        steamRoot = QDir(QDir::home().filePath(".steam/steam"));
        steamFound = true;
    #else
        // TODO?
    #endif
    if (steamFound && steamRoot.exists())
    {
        return steamRoot;
    }
    // Steam was not found, probably not installed.
    return NULL:
}
void SteamFindGames(void) {
      //insert code here
}

This was mentioned before internally, however we decided that, because only the Library will need to call these functions, they belong to the Library class. Creating entire files/classes(2 files each if you do the standard practice) dedicated to 2 functions that don't need to be separated and are going to be refactored out isn't the best way of decreasing code complexity. If you use an editor with a navigate to symbol button of some sort, the "messiness" of the code is more of a non-issue.