Skip to content

Add map and map_with_key Methods to Map #2210

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

Closed
wants to merge 36 commits into from

Conversation

Asterless
Copy link
Contributor

Summary

Based on #1890
This PR adds two new methods to Map in builtin/linked_hash_map.mbt:

  • map(f: (V) -> V2) -> Map[K, V2]
    Applies a function to each value in the map and returns a new map with the same keys and transformed values.

  • map_with_key(f: (K, V) -> V2) -> Map[K, V2]
    Applies a function to each key-value pair and returns a new map with the same keys and transformed values.

Implementation Details

  • Both methods preserve the insertion order of keys.
  • map is implemented by calling map_with_key and ignoring the key.
  • If the map is empty, a new empty map is returned.
  • The new map is pre-allocated with the same size as the original for efficiency.

Example Usage

let m = {1: 10, 2: 20}
let m2 = m.map(fn(v) { v * 2 }) // {1: 20, 2: 40}
let m3 = m.map_with_key(fn(k, v) { k + v }) // {1: 11, 2: 22}

Impact

  • Backward compatible; does not affect existing APIs or behavior.
  • Only adds new functionality to Map.

Asterless and others added 28 commits May 22, 2025 03:06
Add set operations to @immut/hash{map, set} and @internal/sparse_array Summary and re-fmt
Copy link

peter-jerry-ye-code-review bot commented Jun 5, 2025

Pre-allocation optimization missing for partition method

Category
Performance
Code Snippet
let m1 = Map::new(capacity=self.size)
let m2 = Map::new(capacity=self.size)
Recommendation
Consider using a smaller initial capacity for both maps, since the final size of each will likely be less than the original. Something like: let m1 = Map::new(capacity=self.size / 2)
Reasoning
While pre-allocating with the full size works, it's likely wasteful since after partitioning each resulting map will be smaller than the original. This leads to unnecessary memory allocation.

Fold method documentation could be more descriptive

Category
Maintainability
Code Snippet
/// Folds the map from an initial value by applying a function to each key-value pair and the accumulator.
/// The order of folding is not guaranteed.
Recommendation
Add examples in the documentation and clarify the accumulator type requirements:

/// Folds the map from an initial value by applying a function to each key-value pair and the accumulator.
/// The order of folding is not guaranteed.
/// Example:
/// ```
/// let m = {1: 10, 2: 20}
/// let sum = m.fold(0, fn(acc, k, v) { acc + v }) // 30
/// ```
**Reasoning**
More detailed documentation with examples helps users understand the purpose and usage of the method better, especially for functional programming concepts like fold.

</details>
<details>

<summary> Implicit return in for loop could be made explicit </summary>

**Category**
Correctness
**Code Snippet**
for e in self {
  acc = f(acc, e.0, e.1)
}
acc
**Recommendation**
Consider making the mutation and return more explicit:
```moonbit
let mut acc = init
for e in self {
  acc = f(acc, e.0, e.1)
}
return acc

Reasoning
While both versions are functionally equivalent, making the mutation and return explicit improves code readability and makes the control flow more obvious.

@coveralls
Copy link
Collaborator

coveralls commented Jun 5, 2025

Pull Request Test Coverage Report for Build 7130

Details

  • 0 of 20 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 92.933%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/linked_hash_map.mbt 0 20 0.0%
Totals Coverage Status
Change from base Build 7124: -0.2%
Covered Lines: 8574
Relevant Lines: 9226

💛 - Coveralls

@FlyCloudC
Copy link
Contributor

There are many hash calculations that can be avoided here.

Comment on lines 260 to 261
fn[K : Hash + Eq, V] Map::filter(Self[K, V], (K, V) -> Bool?Error) -> Self[K, V]?Error
fn[K : Hash + Eq, V, A] Map::fold(Self[K, V], A, (A, K, V) -> A?Error) -> A?Error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please focus on the functionalities that you described in the introduction; do not add too many stuff in one PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok👍

@peter-jerry-ye
Copy link
Collaborator

There are too many unrelated modifications. I'm closing this PR for the new one.

@peter-jerry-ye
Copy link
Collaborator

Continue in #2228 #2229

@Asterless Asterless deleted the feature/map-enhancement branch June 10, 2025 10:47
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.

4 participants