Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: @immut/array.make and @immut/array.makei #1765

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illusory0x0
Copy link
Contributor

@illusory0x0 illusory0x0 commented Mar 10, 2025

avoid use closure

And this implementation will be a bit simpler than the previous new_by_leaves, and can also be reused by from_iter

@illusory0x0 illusory0x0 force-pushed the refactor-immut_array_make branch from a1e7fcb to 0ed43d5 Compare March 10, 2025 21:26
@illusory0x0 illusory0x0 changed the title refactor: @immut/array.T::make and @immut/array.T::makei refactor: @immut/array.make and @immut/array.makei Mar 10, 2025
@coveralls
Copy link
Collaborator

coveralls commented Mar 10, 2025

Pull Request Test Coverage Report for Build 5695

Details

  • 21 of 22 (95.45%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 92.635%

Changes Missing Coverage Covered Lines Changed/Added Lines %
immut/array/array.mbt 21 22 95.45%
Totals Coverage Status
Change from base Build 5689: -0.007%
Covered Lines: 6037
Relevant Lines: 6517

💛 - Coveralls

Comment on lines +31 to +36
Array::make(quot, FixedArray::make(branching_factor, value))
} else {
let arr : Array[FixedArray[A]] = Array::make(quot + 1, [])
for k in 0..<quot {
arr[k] = FixedArray::make(branching_factor, value)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the alias here expected (using Array::make)? If yes, should it apply to the second arr too?

let arr : Array[FixedArray[A]] = Array::make(quot + 1, FixedArray::make(branching_factor, value))
arr[quot] = FixedArray::make(rem, value)

@peter-jerry-ye peter-jerry-ye force-pushed the refactor-immut_array_make branch from 0ed43d5 to 44a720a Compare March 14, 2025 09:56
Copy link

‼️ This code review is generated by a bot. Please verify the content before trusting it.

The pull request aims to simplify the implementation of make and makei by avoiding closures and reusing the from_leaves function. The changes primarily involve refactoring the code to remove the new_by_leaves function and replacing it with a more direct approach. Here are the key observations and feedback:

💡 [Improvement in code clarity and simplicity]
  • Category: Maintainability
  • Code Snippet: Lines 25-47 (make function), Lines 50-72 (makei function)
  • Recommendation: The new implementation is more straightforward and avoids the use of closures, making the code easier to understand and maintain.
  • Reasoning: By breaking down the problem into simpler steps and avoiding closures, the code becomes more explicit. This reduces the cognitive load on developers and makes the codebase easier to maintain and debug.
⚠️ [Deprecated function `check_fixedarray_eq`]
  • Category: Correctness
  • Code Snippet: Lines 132-140 (check_fixedarray_eq function)
  • Recommendation: If check_fixedarray_eq is deprecated or no longer needed, it should be removed entirely rather than commented out.
  • Reasoning: Commented-out code can clutter the codebase and confuse developers. If the function is no longer used, it should be deleted to keep the codebase clean and maintainable.
💡 [Improvement in function reusability]
  • Category: Maintainability
  • Code Snippet: Lines 362-389 (from_leaves function)
  • Recommendation: The from_leaves function is now reusable and more generic, which is a positive change.
  • Reasoning: By making from_leaves a standalone function that can be used by both make and makei, the code becomes more modular and reusable. This reduces code duplication and promotes better code organization.

Overall, the changes improve the code's readability, maintainability, and reusability. The only significant concern is the commented-out check_fixedarray_eq function, which should be removed if it is no longer needed.

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.

3 participants