refactor: move static Combat methods out of header #5072
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Combat class by moving three static methods (DamageToConditionType, ConditionToDamageType, and isProtected) and one private static method (combatTileEffects) out of the class header and into file-local scope. The goal is to reduce header bloat and limit these functions to the translation units where they are actually used.
Key changes:
DamageToConditionTypemoved fromCombatclass to file-local scope inmap.cppConditionToDamageTypemoved fromCombatclass to file-local scope incondition.cppisProtectedandcombatTileEffectsconverted to file-local functions incombat.cpp
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/combat.h | Removed declarations for isProtected, ConditionToDamageType, DamageToConditionType, and combatTileEffects from Combat class |
| src/combat.cpp | Converted isProtected and combatTileEffects to file-local static functions; removed implementations of ConditionToDamageType and DamageToConditionType |
| src/map.cpp | Added file-local DamageToConditionType function; updated call site to use function without Combat:: prefix |
| src/condition.cpp | Added file-local ConditionToDamageType function; updated call site to use function without Combat:: prefix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return MAP_NORMALWALKCOST; | ||
| } | ||
|
|
||
| constexpr ConditionType_t DamageToConditionType(CombatType_t type) |
There was a problem hiding this comment.
The function DamageToConditionType should be placed in an anonymous namespace or be declared as static to follow the codebase's convention for file-local helper functions. The codebase uses anonymous namespaces for file-local functions (e.g., see lines 522-558 in map.cpp with checkSteepLine and checkSlightLine functions).
| return false; | ||
| } | ||
|
|
||
| constexpr CombatType_t ConditionToDamageType(ConditionType_t type) |
There was a problem hiding this comment.
The function ConditionToDamageType should be placed in an anonymous namespace or be declared as static to follow the codebase's convention for file-local helper functions. This would prevent potential naming conflicts and clearly indicate that this function has internal linkage.
ramon-bernardo
left a comment
ramon-bernardo
left a comment
There was a problem hiding this comment.
I see that there are still some static methods, wouldn't it be possible to move to local namespace?
No, they are used elsewhere. Could be moved to a namespace but I would do it separately |
2 participants
Pull Request Prelude
Changes Proposed
Moves static methods that are only used once out of the header file to avoid bloating the class namespace.