Skip to content

Refactor earnings calculations in WorkdayService and CompensationCalculator to use BigDecimal for precision and apply consistent rounding#45

Merged
jeyongsong merged 2 commits intomainfrom
fix/monthly-earnings-roundoff
Apr 3, 2026
Merged

Refactor earnings calculations in WorkdayService and CompensationCalculator to use BigDecimal for precision and apply consistent rounding#45
jeyongsong merged 2 commits intomainfrom
fix/monthly-earnings-roundoff

Conversation

@jeyongsong
Copy link
Copy Markdown
Member

This pull request refines the way earnings are calculated and rounded in the compensation logic, ensuring greater precision and consistency between daily and monthly totals. The main changes involve switching from integer-based to BigDecimal-based calculations for earnings, delaying rounding until the monthly total is computed, and updating tests to reflect these changes.

Earnings Calculation and Rounding Improvements:

  • Changed the accumulation of workedEarnings in WorkdayService from Long to BigDecimal, and now only rounds to the nearest integer when returning the monthly total. This prevents rounding errors from accumulating day-by-day. [1] [2]
  • Updated daily earnings calculation to use BigDecimal and round only at the point of returning integer results, ensuring consistency in rounding logic.
  • Introduced a constant MONEY_SCALE in CompensationCalculator to standardize decimal precision in monetary calculations.
  • Modified calculation methods in CompensationCalculator to use MONEY_SCALE for division and to avoid premature rounding, improving accuracy. [1] [2]

Test Updates:

  • Updated compensation calculation tests to expect precise BigDecimal values instead of rounded integers, reflecting the new calculation logic. [1] [2] [3] [4] [5] [6] [7] [8] [9]
  • Added a new test to verify that rounding only at the monthly total preserves the expected monthly sum, preventing cumulative rounding errors.

Other:

  • Added necessary imports for RoundingMode in relevant files. [1] [2]

…ulator to use BigDecimal for precision and apply consistent rounding
Copilot AI review requested due to automatic review settings April 3, 2026 09:36
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

Test Results

47 tests   47 ✅  0s ⏱️
 6 suites   0 💤
 6 files     0 ❌

Results for commit 1df3762.

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors compensation/earnings calculations to use BigDecimal end-to-end for higher precision, standardizes intermediate division precision, and defers rounding until returning monthly totals to avoid cumulative day-by-day rounding drift.

Changes:

  • Accumulate monthly workedEarnings as BigDecimal in WorkdayService and round only once when returning the monthly total.
  • Standardize monetary division precision in CompensationCalculator via MONEY_SCALE, and avoid premature rounding in calculateEarnings.
  • Update calculator tests to assert precise BigDecimal outputs and add a regression test to validate “round only at monthly total” behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/main/kotlin/com/moa/service/WorkdayService.kt Switch monthly earnings accumulation to BigDecimal and apply consistent rounding at response boundaries.
src/main/kotlin/com/moa/service/calculator/CompensationCalculator.kt Introduce MONEY_SCALE and adjust daily-rate / earnings calculations to preserve precision until later rounding.
src/test/kotlin/com/moa/service/calculator/CompensationCalculatorTest.kt Update expected values to high-precision BigDecimal and add a monthly-sum rounding test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…le exact policy work minutes and update test cases for accuracy
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 176 to +180
): BigDecimal {
if (policyWorkMinutes <= 0) return dailyRate
val minuteRate = dailyRate.divide(BigDecimal(policyWorkMinutes), 10, RoundingMode.HALF_UP)
return minuteRate.multiply(BigDecimal(actualWorkMinutes)).setScale(0, RoundingMode.HALF_UP)
if (actualWorkMinutes == policyWorkMinutes) return dailyRate
val minuteRate = dailyRate.divide(BigDecimal(policyWorkMinutes), MONEY_SCALE, RoundingMode.HALF_UP)
return minuteRate.multiply(BigDecimal(actualWorkMinutes))
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calculateEarnings returns dailyRate directly when actualWorkMinutes == policyWorkMinutes, but returns a multiplied value (with MONEY_SCALE decimals) otherwise. Because BigDecimal equality is scale-sensitive (e.g., 100000 != 100000.0000000000), this introduces inconsistent scales depending on the branch, which can cause subtle bugs if callers use equals/hashing or serialize values. Consider normalizing the scale of the returned value (e.g., always setScale(MONEY_SCALE, ...) or stripTrailingZeros() consistently) before returning.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BigDecimal scale inconsistency point is valid in general, but in this flow values are normalized at response boundaries via setScale(0, HALF_UP), and comparisons use compareTo semantics. So it should not affect current behavior. I’d prefer to avoid additional intermediate normalization here because that would reintroduce rounding into the calculation path we just fixed.

@jeyongsong jeyongsong merged commit c91118a into main Apr 3, 2026
7 checks passed
@jeyongsong jeyongsong deleted the fix/monthly-earnings-roundoff branch April 3, 2026 09:49
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.

2 participants