Maintenance/code smells - #28
Conversation
These are local Claude Code context docs, not meant to ship in the PR. Removes them from git tracking (files remain on disk) and ignores CLAUDE.md/INDEX.md going forward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2TLHGAcKimciZBzgcVMnN
Native audio management handles the soundId associated with each SoundEvent.
| // PROPOSED (CS0114, should add `new` or `override`): StatusEffectMissionLogic.cs calls this | ||
| // through the concrete `StatusEffectComponent` type (not polymorphically via | ||
| // `AgentComponent`), so `new` would match current behavior. Before adding `override` instead, | ||
| // worth checking whether the engine also auto-invokes AgentComponent.OnTick on every |
There was a problem hiding this comment.
Added a clarifying remark for why we can't use the OnTick override that's available via AgentComponent and renamed the method to better differentiate between the seeming "Tick" calls.
| public static bool ContainsSpellType(AbilityComponent component, int spellCount, AbilityEffectType excludedEffectType) | ||
| { | ||
| var wrongSpell = false; | ||
| //TODO: Why not use linq? |
There was a problem hiding this comment.
I once stumbled upon a comment from TW, I think, which recommended using indexed for loops instead of foreach loops f.e. because the former were more performant.
There wasn't much context on the circumstances in which they were more performant, but this may be an implementation of a similar thought process.
Idk if there's any performance benefit or cost to LINQ, but it would probably be slightly easier to read intention.
On the otherhand, I'm thoroughly confused as to why GetAbility(i) performs a modulo on the passed index as having circular indexing seems useless when the iterations are all limiting themselves to .Count.
There's derivative uses that don't check count, but they're pulling the useable AbilityTemplates from the hero's selected ability list in which case the index must necessarily exist within the bounds of the list.
Rabbit hole for another day when the ability templates get a rewrite.
|
I've changed everything relevant and added clarifications where useful. I'll let you handle the merge conflict. Afaict, it's to override the next_update EnchantmentHelper contents with the contents of this branch, but it's too late for the brain to be confident in that. |
|
Branch is ready for merge. Leaving some time for naber to respond about the party food, but from what I see it can be removed completely along with usages of it. |
3 participants
Various code smells, please note that I wasn't able to fix them all directly, as they require some context around either the battle engine or otherwise.