Skip to content

Simplify kth_factor implementation - #1069

Open
Watagon wants to merge 2 commits into
TheAlgorithms:masterfrom
Watagon:improve/kth_factor
Open

Watagon wants to merge 2 commits into
TheAlgorithms:masterfrom
Watagon:improve/kth_factor

Conversation

@Watagon

@Watagon Watagon commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Description

This pull request refactors kth_factor to use Rust iterator methods instead of allocating and manually searching a Vec<i32>.

The implementation iterates over the candidate factors, keeps the values that divide n evenly, selects the (k - 1)th matching value, and returns -1 when no such factor exists. The existing behavior and test cases are preserved.

I would appreciate the maintainers' guidance on two API questions:

  1. Should the function explicitly handle k == 0? The current implementation assumes a one-based factor index, and k == 0 currently causes an unsigned subtraction underflow in debug builds.
  2. Should the return type remain i32 with -1 indicating that the kth factor does not exist, as in common implementations of this problem, or should it be changed to the more idiomatic Rust type Option<i32>?

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I ran the commands using the latest version of rust nightly.
  • I ran cargo clippy --all -- -D warnings just before my last commit and fixed any issue that was found.
    • The command was run and fixed some issues, but it currently fails on 7 pre-existing clippy warnings and they are unfixable.
    • Details in here
  • I ran cargo fmt just before my last commit.
  • I ran cargo test just before my last commit and all tests passed.
  • I added my algorithm to the corresponding mod.rs file within its own folder, and in any parent folder(s).
  • I added my algorithm to DIRECTORY.md with the correct link.
  • I checked CONTRIBUTING.md and my code follows its guidelines.

Please make sure that if there is a test that takes too long to run ( > 300ms), you #[ignore] that or try to optimize your code or make the test easier to run. We have this rule because we have hundreds of tests to run; If each one of them took 300ms, we would have to wait for a long time.

The clippy warnings

I'm getting the runaround.

The new rule clippy::rest-pattern-accessible-field is causing problems.

An error before the fix:

error: struct destructuring with rest (`..`)
  --> src/compression/huffman_encoding.rs:66:13
   |
66 |             HuffmanNode::Leaf { frequency, .. } | HuffmanNode::Internal { frequency, .. } => {
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/main/index.html#rest_pattern_accessible_field
   = note: `-D clippy::rest-pattern-accessible-field` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::rest_pattern_accessible_field)]`
help: consider explicitly ignoring remaining fields with wildcard patterns (`x: _`)
   |
66 -             HuffmanNode::Leaf { frequency, .. } | HuffmanNode::Internal { frequency, .. } => {
66 +             HuffmanNode::Leaf { frequency, character: _ } | HuffmanNode::Internal { frequency, .. } => {
   |

After fix it following the help:

error: you matched a field with a wildcard pattern, consider using `..` instead
  --> src/compression/huffman_encoding.rs:66:44
   |
66 |             HuffmanNode::Leaf { frequency, character: _ } | HuffmanNode::Internal { frequency, left: _, right: _ } => {
   |                                            ^^^^^^^^^^^^
   |
   = help: try with `Leaf { frequency, .. }`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/main/index.html#unneeded_field_pattern
   = note: `-D clippy::unneeded-field-pattern` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::unneeded_field_pattern)]`

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.88%. Comparing base (2345c66) to head (8093a27).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1069      +/-   ##
==========================================
- Coverage   95.89%   95.88%   -0.01%     
==========================================
  Files         396      396              
  Lines       30440    30431       -9     
==========================================
- Hits        29191    29180      -11     
- Misses       1249     1251       +2     

☔ View full report in Codecov by Harness.
📢 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.

@Watagon

Watagon commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Should I disable clippy::rest-pattern-accessible-field?

@Watagon
Watagon marked this pull request as ready for review September 16, 2026 04:34
@Watagon
Watagon requested a review from imp2002 as a code owner September 16, 2026 04:34
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