Skip to content

fix: include number/2 divisor in FriendlyNumbers sumDivisors#1904

Open
shaked-shlomo wants to merge 1 commit into
TheAlgorithms:masterfrom
shaked-shlomo:fix/friendly-numbers-divisor-sum
Open

fix: include number/2 divisor in FriendlyNumbers sumDivisors#1904
shaked-shlomo wants to merge 1 commit into
TheAlgorithms:masterfrom
shaked-shlomo:fix/friendly-numbers-divisor-sum

Conversation

@shaked-shlomo
Copy link
Copy Markdown

Bug

Maths/FriendlyNumbers.js's sumDivisors uses an exclusive upper bound:

for (let i = 0; i < number / 2; i++) {

This excludes i === number / 2, which is a divisor of every even number, so the sum of divisors (and therefore the abundancy index σ(n)/n) is wrong for even inputs:

  • sumDivisors(6) returns 9 instead of 12
  • sumDivisors(28) returns 42 instead of 56

Because the error scales both numbers similarly it sometimes cancels out (the classic FriendlyNumbers(6, 28) still returns true by luck), but it produces wrong classifications in general — e.g. FriendlyNumbers(15, 20) returns true even though σ(15)/15 = 1.6 ≠ σ(20)/20 = 2.1.

Fix

Loop from 1 to number / 2 inclusive:

for (let i = 1; i <= number / 2; i++) {

(Starting at 1 also drops the harmless number / 0 = Infinity check the old i = 0 start relied on.)

After the fix: sumDivisors(6) = 12, sumDivisors(28) = 56, FriendlyNumbers(6, 28) = true, FriendlyNumbers(30, 140) = true, FriendlyNumbers(15, 20) = false.

The module had no test file, so I added Maths/test/FriendlyNumbers.test.js covering friendly pairs, a non-friendly pair, and invalid input.

sumDivisors looped 'for (let i = 0; i < number / 2; i++)', which excludes
i === number/2 -- a real divisor of every even number -- so the divisor
sum (and abundancy index) was wrong for even inputs, e.g. sumDivisors(6)
returned 9 instead of 12 and sumDivisors(28) returned 42 instead of 56.
That made FriendlyNumbers misclassify pairs (e.g. FriendlyNumbers(15, 20)
returned true though their abundancy indices differ).

Loop from 1 to number/2 inclusive. Added a test file (the module had none).
@shaked-shlomo shaked-shlomo requested a review from appgurueu as a code owner June 1, 2026 23:51
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.11%. Comparing base (5c39e87) to head (3dd22dd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1904      +/-   ##
==========================================
+ Coverage   85.91%   86.11%   +0.19%     
==========================================
  Files         379      379              
  Lines       19778    19778              
  Branches     3016     3024       +8     
==========================================
+ Hits        16993    17031      +38     
+ Misses       2785     2747      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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