Codebase refactor
dongresource opened this issue · 2 comments
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.
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:
- AI.cpp (or perhaps call it MobAI.cpp, depending on how much escort/friendly/scriptable logic ends up in here)
- 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.cppAbilities.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:
- BuiltinCommands.cpp for all the GM-only text commands
- 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:
- Trading.cpp
-
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:
- The commands should be moved into CustomCommands.cpp
- 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.
Same comment as the other issue, how much of this is left to clean up post-refactor branch merge?