Codebase refactor

[dongresource]

Up until now we've been trying to keep the number of source files low because of concerns over C++'s inefficient handling of multiple units of compilation, specifically when hierarchies of included header files are involved. It's become clear that this isn't worth the trade-off anymore, because the arbitrary grouping of functionality into preexisting files has made the codebase difficult to navigate.

Apart from redistributing logic across more source files, we'll also take this opportunity to reduce the use of the suffix "Manager" in the various namespace identifiers, as it's something of an anti-pattern because it adds very little meaning yet makes them more difficult to read and write. We don't necessarily need to remove its use from all existing namespaces, but we should avoid using it in new ones. It should be removed from some existing namespaces, though.

Another thing we should do is cleanly separate internal and external functions and data, instead of exporting everything in the corresponding header file.

For example, MissionManager::endTask() is currently defined in MissionManager.cpp and declared in the MissionManager namespace in MissionManager.hpp.

In MissionManager.hpp:

namespace MissionManager {
    // ...
    bool endTask(CNSocket *sock, int32_t taskNum, int choice=0);
    // ...
}

In MissionManager.cpp:

bool MissionManager::endTask(CNSocket *sock, int32_t taskNum, int choice) {
    // ...
}

The proper way to go about it is to not put the function in a namespace at all, and to instead both define and declare it with the static keyword, and the declaration should be put near the top of the source file, after the global variables but before the function definitions, instead of in the header file. Like so:

static bool endTask(CNSocket *sock, int32_t taskNum, int choice=0);

// other function definitions...

static bool endTask(CNSocket *sock, int32_t taskNum, int choice) {
    // ...
}

Internal global variables should have static in their definition near the top of the source file and shouldn't be declared anywhere else.

[dongresource]

This is a rough outline of how I'd like to restructure the codebase.

The current raw lines-of-code count for the largest few source files is:

  2045 src/MobManager.cpp
  1981 src/Database.cpp
  1294 src/PlayerManager.cpp
  1131 src/ItemManager.cpp
  1122 src/ChatManager.cpp
  1067 src/TableData.cpp
   994 src/NPCManager.cpp
   946 src/Defines.hpp
   924 src/NanoManager.cpp
   891 src/BuddyManager.cpp
   650 src/CNLoginServer.cpp
   612 src/MissionManager.cpp
   534 src/ChunkManager.cpp

MobManager currently contains routines related to basic combat (including things like goo damage), mob AI, player update ticks, mob powers, mob drops and event crates. It should be split into:

• [x] AI.cpp (or perhaps call it MobAI.cpp, depending on how much escort/friendly/scriptable logic ends up in here)
• [x] Combat.cpp (crate dropping logic can probably go in here as well)
• [ ] Possibly a PlayerState.cpp, if we decide that putting the player ticking logic in Combat.cpp isn't appropriate
• [ ] MobPowers.cpp (or possibly aPowers.cpp Abilities.cpp, if we decide to lump them in with Nano powers)

Database.cpp is the file most in need of static functions, since there's a clear distinction between those that must be locked and those that mustn't. As for separation; the logic is somewhat self-contained, so we could either keep it as the single largest file or split it up into a few files kept in their own db/ subdirectory.

PlayerManager should be split into:

• [x] BuiltinCommands.cpp for all the GM-only text commands
• [x] PlayerMovement.cpp for the trivial handlers of the various only-slightly-distinct movement packets and perhaps their underlying support functions. Maybe the warp functions as well.
• The rudimentary/misc/helper functions like addPlayer()/removePlayer(), enterPlayer(), sendToViewable(), etc, should stay in PlayerManager.cpp. It can also keep the "Manager" part of its name, I think.

ItemManager should be split into:

• [x] Trading.cpp
• [x] itemGMGiveHandler() can go into BuiltinCommands.cpp
• Most of the other stuff can stay in ItemManager
• Helper functions for sItemBase comparison, clearing, null-checking, validation and stackable/combined checking should be put in this source file

ChatManager:

• [x] The commands should be moved into CustomCommands.cpp
• [x] The other chat handlers (group, buddy, trade) could be moved into ChatManager proper

TableData is a thankfully self-contained mess that will hopefully be dealt with sometime later.

NPCManager.cpp should probably become Vendor.cpp, and the unrelated stuff moved into one of the other general-ish files, like PlayerState.cpp or PlayerManager.cpp or Combat.cpp, or whichever combination of those ends up existing. Eggs.cpp can be a thing too, maybe.

The Nano power functions should be moved into NanoPowers.cpp (or Powers.cpp Abilites; see above). The rest is good as is.

MissionManager would benefit from more helper functions and far less nested inline loops and conditionals.

Some of the logic in ChunkManager will naturally become less redundant if we manage to handle Players, NPCs, Sliders and Eggs uniformly, through a thin shared superclass/interface.

Player.hpp and NPC.hpp could be integrated into the headers of their respective places of belonging, though maybe Player.hpp should remain separate, as it's sort of a Grand Unifying Structure or whatever? Not sure.

And that's about it.

[yungcomputerchair]

Same comment as the other issue, how much of this is left to clean up post-refactor branch merge?