Skip to content

The last percentage division without a guard - #2310

Open
denislauri1999 wants to merge 1 commit into
NosCoreIO:masterfrom
denislauri1999:pr/stat-percentage-guard
Open

The last percentage division without a guard#2310
denislauri1999 wants to merge 1 commit into
NosCoreIO:masterfrom
denislauri1999:pr/stat-percentage-guard

Conversation

@denislauri1999

@denislauri1999 denislauri1999 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

GenerateStatInfo divides by MaxHp and MaxMp with nothing checking them, while every other place that computes the same two percentages checks first — including one five hundred lines up in the same file:

// PlayerBundleExtensions.cs:238
Hp = player.MaxHp > 0 ? (int)(player.Hp / (float)player.MaxHp * 100) : 100,

// PlayerBundleExtensions.cs:773
HpPercentage = (int)(player.Hp / (float)player.MaxHp * 100),

It never threw and never would. The division is on floats, so a maximum of zero gives NaN rather than an exception, and the cast then puts a meaningless number in the two fields the client draws as the health and mana bars. No log, no symptom to search for.

I swept the rest rather than fixing only what I tripped over

Every other float division of this shape in src/ is guarded:

AliveEntityExtension 102-106 MaxHp > 0 ? … : 100
MonsterBundleExtensions 33 MaxHp > 0 ? … : 100
NpcBundleExtensions 33 MaxHp > 0 ? … : 100
VisualEntityExtension 36 ?? 1
PlayerBundleExtensions 238-239 MaxHp > 0 ? … : 100
DamageCalculator.RollBaseDamage returns early on max <= min
RewardService 98 caller returns on totalDamage <= 0

GenerateStatInfo was the only one left.

The fallback of 100 is the one the guarded computation in the same file already uses, rather than inventing a second convention for the same question.

Removing the guard fails one of the three tests.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented health and mana displays from showing invalid values when their maximum is zero.
    • Zero-maximum stats now display as 100% instead of causing an invalid calculation.
    • Preserved the existing percentage calculations for stats with valid maximum values.
  • Tests

    • Added coverage for normal percentages, zero-maximum handling, and consistency with guarded calculations.

`GenerateStatInfo` divides by MaxHp and MaxMp with nothing checking them, while
every other place that computes the same two percentages checks first -
including one five hundred lines up in the same file.

It never threw and never would: the division is on floats, so a maximum of zero
gives NaN rather than an exception, and the cast then puts a meaningless number
in the two fields the client draws as the health and mana bars. No log, no
symptom to search for, a bar reading whatever that number happened to be.

I swept the rest rather than fixing only what I tripped over. Every other
float division of this shape in src/ is guarded: AliveEntityExtension,
MonsterBundleExtensions, NpcBundleExtensions, VisualEntityExtension, the earlier
one in this same file, RollBaseDamage in DamageCalculator, and the reward share
in RewardService, whose caller returns early on a total of zero. This was the
only one left.

The fallback of 100 matches what the guarded computation five hundred lines up
already uses, rather than inventing a second convention.

Removing the guard fails one of the three tests.
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e5f2f9d-b2f9-4aed-83c6-21331b09ee22

📥 Commits

Reviewing files that changed from the base of the PR and between 55dc38c and 4a67e5f.

📒 Files selected for processing (2)
  • src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs
  • test/NosCore.GameObject.Tests/Ecs/Extensions/StatInfoPercentageTests.cs

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


Walkthrough

GenerateStatInfo now avoids invalid percentage calculations when maximum health or mana is zero. New tests cover normal percentages, zero-maximum behavior, and consistency with the guarded health formula.

Changes

Stat percentage handling

Layer / File(s) Summary
Guard stat percentages and validate calculations
src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs, test/NosCore.GameObject.Tests/Ecs/Extensions/StatInfoPercentageTests.cs
GenerateStatInfo returns 100 when maximum health or mana is zero. Tests cover ordinary percentages, zero-maximum values, and the guarded health calculation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4a67e

The change guards health and mana percentage calculations against zero maximum values and uses the existing 100% fallback convention, with focused tests covering the behavior. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding a guard to the final unguarded percentage division.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant