☂️ Game engine: Issues with general code organisation
We have some issues with general organisation of code:
- [ ] There are many "god objects" in the code. The most infamous are
game,map,item,Creatureand all its children:monster,Character,player,npc,avatar. Besides the standard problems of "god objects", these classes also form some of the biggest and most included headers we have, which negatively affects compilation times for both clean and incremental builds. Ideally these classes should be broken up, or stripped of methods that can be converted into free functions (especially those methods that only operate on other public methods and those that don't touch the object at all). Some effort has been done on this front, but nothing substantial. - [ ] We try to preserve code compatibility with DDA to make porting updates from there easier, but it's causing a massive block on some tooling (#3118) and code organisation (#236) changes. With 3.5 years worth of divergence, we may want to reconsider this stance, as most attempts at porting end up with conflicts anyway.
- [ ] All source files exist in a single folder
src, with no clean separation for frontend/backend, or purpose, or library they belong to. Moving everything to subdirectories may not be desirable since that'd ruin compatibility with DDA (unless we decide we don't care about that), but some parts at least could be moved out of the way. - [ ] Some functionality is spread out in code in unintuitive ways. For example,
output.hheader contains a mix of string formatting code, generic UI output code, item-specific UI code, UI management code and UI widgets, butcursesdef.halso contains UI code, as well ascursesport.handui.h, butui.halso has widgets and helpers, and there's alsoui_manager.hthat has UI management code, and various other headers such ascalendar.hcan have their own formatting functions. - [ ] Namespaces are underutilized: many globally available functions and constants, mainly ui code, are not sorted out into namespaces while they could be.
- [ ] We have Code Guidelines and Best Practices for C++ code, but neither are enforced for PRs or even hold true for existing code.
- [ ] ~~We have inherited large code migration projects, those will have to be finished eventually: https://github.com/cataclysmbnteam/Cataclysm-BN/issues/318#issuecomment-760100067~~ Moved to #3271
We have inherited large code migration projects, those will have to be finished eventually: #318
Cute to see my 2 year old triple digit issue report mentioned
"Finish eventually" is a little loaded, half the issues I made were caused by temperature being disabled, the other half could have easily been solved by selective reverts in the code, undoing those projects to before they were started. I'd be shocked if those are still in the game, honestly, it's just a revert.
"Finish eventually" was specifically about the issues listed in the comment https://github.com/cataclysmbnteam/Cataclysm-BN/issues/318#issuecomment-760100067 (GitHub UI inserts the issue title, which makes it a bit unclear that one of the comments was linked). Out of those projects, there's no sense in reverting them as they're straight up upgrades over existing architecture. I've moved that comment to #3271 for clarity.
Regarding #318 itself, I've checked and it seems we actually did handle all the issues originally listed there, so I've closed it as completed. If anything new pops up (or old stuff reappears), feel free to open new issue.
but it's causing a massive block on some tooling
i made a JSON query/manipulation API for BN (https://github.com/scarf005/catjazz) and working around this has been painful.
https://github.com/scarf005/catjazz/blob/main/utils/json_fmt.ts