So, I've been doing rust for some time and love (most of it). Though, when refactoring, I always end up fighting the types (especially iterator types) and the borrow checker.
The code below is an example of when I have code repetition that I fail to refactor away. I'm coming from Python and Haskell where I can just wrap everything in functions, but with Rust, it seems not that easy.
So here is the non refactored code with code repetition:
pub fn normalize_pattern_substitution(
pattern: Pattern,
substitution: Pattern,
) -> (Pattern, Pattern) {
let mut index_map = FxHashMap::default();
let mut current_index = 0;
let new_pattern: Pattern = pattern
.into_iter()
.map(|p| {
let [a, l, b] = p;
let na = match index_map.get(&a) {
Some(_a) => *_a,
None => {
index_map.insert(current_index, a);
current_index += 1;
current_index - 1
}
};
let nb = match index_map.get(&b) {
Some(_b) => *_b,
None => {
index_map.insert(current_index, b);
current_index += 1;
current_index - 1
}
};
[na, l, nb]
})
.collect::<Vec<_>>();
let new_substitution: Pattern = substitution
.into_iter()
.map(|p| {
let [a, l, b] = p;
let na = match index_map.get(&a) {
Some(_a) => *_a,
None => {
index_map.insert(current_index, a);
current_index += 1;
current_index - 1
}
};
let nb = match index_map.get(&b) {
Some(_b) => *_b,
None => {
index_map.insert(current_index, b);
current_index += 1;
current_index - 1
}
};
[na, l, nb]
})
.collect::<Vec<_>>();
(new_pattern, new_substitution)
}
My plan was to rewrite this as:
pub fn normalize_pattern_substitution(
pattern: Pattern,
substitution: Pattern,
) -> (Pattern, Pattern) {
macro_rules! get_set {
( $x:expr, $map:expr, $ci:expr) => {
match $map.get(&$x) {
Some(_x) => *_x,
None => {
let i = $ci.clone();
$map.insert($x, i);
$ci += 1;
i
}
}
};
}
macro_rules! norm_pattern {
( $pattern:expr, $map:expr, $ci:expr) => {
$pattern
.into_iter()
.map(|p| {
let [a, l, b] = p;
let na = get_set!(a, $map, $ci);
let nb = get_set!(b, $map, $ci);
[na, l, nb]
})
.collect::<Vec<_>>()
};
}
let mut index_map = FxHashMap::default();
let mut current_index = 0;
let new_pattern: Pattern = norm_pattern!(pattern, index_map, current_index);
let new_substitution: Pattern = norm_pattern!(substitution, index_map, current_index);
(new_pattern, new_substitution)
}
How would a more experienced rust programmer refactor it? Any links or resources are more than welcome for general best rust practices.
I apologize for this vague and unstructured question, but I hope anyone can help me in my search.
Thanks
Solution summary
The biggest problem with code was not the borrow checker.
My mind was locked in writing the code as
fn foo(a: IncrementalMap) -> IncrementalMap {
let mut a = a;
a.get_index_or_increment_insert(5);
a
}
...
pattern.into_iter().map(|p| {
a = foo(a);
});
that cause problem with ownership and iterator. Final refactor looks like below. And as pointed out by Michael-F-Bryan my biggest issue was code clutter (that I will address in my code in the future).
Thanks all for the comments.
struct IncrementalMap {
last_index: usize,
map: FxHashMap<usize, usize>,
}
impl IncrementalMap {
/// If key exist in map return, else map key with current index and increment
fn get_index_or_increment_insert(&mut self, key: usize) -> usize {
let val = self.map.entry(key).or_insert_with(|| self.last_index);
self.last_index += 1;
*val
}
fn new() -> IncrementalMap {
IncrementalMap {
last_index: 0,
map: FxHashMap::default(),
}
}
}
pub fn normalize_pattern_substitution(
pattern: Pattern,
substitution: Pattern,
) -> (Pattern, Pattern) {
fn norm_pattern(pattern: &Pattern, incremental_map: &mut IncrementalMap) -> Pattern {
pattern
.into_iter()
.map(|p| {
let [a, l, b] = p;
let new_a = incremental_map.get_index_or_increment_insert(*a);
let new_b = incremental_map.get_index_or_increment_insert(*b);
[new_a, *l, new_b]
})
.collect_vec()
}
let mut incremental_map = IncrementalMap::new();
let new_pattern: Pattern = norm_pattern(&pattern, &mut incremental_map);
let new_substitution: Pattern = norm_pattern(&substitution, &mut incremental_map);
(new_pattern, new_substitution)
}
