Finding the maximum number in a 2D vector

Question

Several people who work with Rust have said that if you find yourself using a lot of clones, you're probably doing something wrong. I'm trying to rewrite some code I wrote to find the maximum number in a 2D vector to use RefCells for the inner vector due to the fact that (as much as I hate it) I need interior mutability on the 2D vec for my physics engine. However, all my code related to this now needs a ton of clone calls. I'm using the "maximum integer" part of my codebase just because its the shortest and easiest to explain piece. Here is my code:

let max_row = |row: &RefCell<Vec<Unit>>| -> Option<usize> {
 row.clone()
 .into_inner()
 .iter()
 .fold(None, |max, unit: &Unit| match max {
 None => Some(unit.tiles.len()),
 el @ Some(_) => {
 if el.unwrap() < unit.tiles.len() {
 Some(unit.tiles.len())
 } else {
 el
 }
 }
 })
};
// find the highest height in this list of rows. Same as above, but uses above. (:
let toplevel = world.map
 .iter()
 .fold(None, |max, vec| match max {
 None => max_row(vec),
 r @ Some(_) => {
 match max_row(vec) {
 None => r,
 z @ Some(_) => {
 if r.unwrap() < z.unwrap() { z } else { r }
 }
 }
 }
});

This is a tad ugly, but the worst part is the use of .clone and .into_inner right in a row after sending it as a pointer. For some reason, it just feels icky, probably because I did that as a fix, not how I originally was hoping it could turn out. I have only a mediocre understanding of borrowing, which could be the problem.

To clarify the code, here is Unit:

#[derive(Clone)]
pub struct Unit {
 pub tiles: Vec<Tile>,
}
// And a basic version of Tile
#[derive(Copy, Clone)]
pub enum Tile {
 Wall,
 Floor,
}

And world is just a Vec<RefCell<Vec<Unit>>>.

I really hope there is a better way, and if there is, could you explain it in such a way that I could apply it to other ugly areas of my codebase where this is also a problem? For instance, I have another area where I need to use a vector from inside a closure, but also need to modify it later, so in the closure I am cloning it, which seems like an ugly solution.

• Shepmaster: Please review your code and try to produce a compilable example. Trying to compile the code you've provided errors with error: no field `map` on type `std::vec::Vec<std::cell::RefCell<std::vec::Vec<Unit>>>`

Answer 1

Normally, I start by walking through the code and improving it step-by-step, but this time I'll start with one piece of advice and showing my final solution. Read the Iterator docs and commit the method names and a rough idea of what they do to memory!

let max = world.iter().flat_map(|r| {
 let r = r.borrow();
 r.iter().map(|unit| unit.tiles.len()).max()
}).max();

---

1. Instead of matching, seeing if something is a Some, then unwrapping it, just bind the pattern to a variable:

   match max {
    None => max_row(vec),
    Some(old_max) => {
    match max_row(vec) {
    None => Some(old_max),
    Some(new_max) => {
    if old_max < new_max {
    Some(new_max)
    } else {
    Some(old_max)
    }
    }
    }
    }
   }
   

2. Extract that max_row call to before the match, since it's done unconditionally.

3. Getting a default from an Option is a common pattern: Option::unwrap_or and Option::unwrap_or_else. Converting an Option<T> to another Option<U> , but only if it's Some, is also a common pattern: Option::map and Option::and_then. You should also memorize what capabilities Option and Result have.

   let mr = max_row(vec);
   max.map(|old_max| {
    match mr {
    None => Some(old_max),
    Some(new_max) => {
    if old_max < new_max {
    Some(new_max)
    } else {
    Some(old_max)
    }
    }
    }
   }).unwrap_or(mr)
   

4. Repeat.

   max.map(|old_max| {
    mr.map(|new_max| {
    if old_max < new_max {
    Some(new_max)
    } else {
    Some(old_max)
    }
    }).unwrap_or(Some(old_max))
   }).unwrap_or(mr)
   

5. map(F).unwrap_or(D) is also a common pattern: Option::map_or:

   max.map_or(mr, |old_max| {
    mr.map_or(Some(old_max), |new_max| {
    if old_max < new_max {
    Some(new_max)
    } else {
    Some(old_max)
    }
    })
   })
   

6. I don't know how to incrementally get out of this code. One possibility is to switch to a match statement to try and flatten some of the cases:

   match (max, max_row(vec)) {
    (None, new_max) => new_max,
    (Some(old_max), None) => Some(old_max),
    (Some(old_max), Some(new_max)) => {
    if new_max > old_max {
    Some(new_max)
    } else {
    Some(old_max)
    }
    }
   }
   

7. std::cmp::max exists, so I'd use that:

   match (max, max_row(vec)) {
    (None, new_max) => new_max,
    (Some(old_max), None) => Some(old_max),
    (Some(old_max), Some(new_max)) => Some(cmp::max(new_max, old_max)),
   }
   

8. Again, I don't know a way to incrementally get from here to iterator methods. This is why I encourage everyone to memorize what functions exist so that you can read the iterator documentation when you know there's something that solves a problem.