The Locker Problem is as followed:
Twenty bored students take turns walking down a hall that contains a row of closed lockers, numbered 1 to 20. The first student opens all the lockers; the second student closes all the lockers numbered 2, 4, 6, 8, 10, 12, 14, 16, 18, 20; the third student operates on the lockers numbered 3, 6, 9, 12, 15, 18: if a locker was closed, he opens it, and if a locker was open, he closes it; and so on. For the ith student, he works on the lockers numbered by multiples of i: if a locker was closed, he opens it, and if a locker was open, he closes it. What is the number of the lockers that remain open after all the students finish their walks?
I decided to take this problem the extra mile and instead allow the user determine how many students were in the hallway so the number isn't always 20. In what ways can I refactor my code?
use std::io;
fn main() {
// false = closed
// true = opened
let mut lockercount = String::new();
println!("How many students?");
io::stdin().read_line(&mut lockercount).expect("expected int");
let lc = lockercount.trim().parse().unwrap();
let mut lockers = vec![false; lc];
for i in 1..(lockers.len()+1) {
let mut j: usize = 1;
while i*j <= lc {
lockers[(i*j)-1] = !lockers[(i*j)-1];
j += 1;
}
}
let mut lockernum = 1;
println!("Opened lockers:");
for v in lockers {
if v == true {
print!("{}, ", lockernum);
}
lockernum += 1;
}
}
1 Answer 1
Your code seems fine. However, there is one part I'd definitely change: the scope of variables. For example, lockercount
isn't necessary for more than the initial lc
. We can keep its scope down:
println!("How many students?");
let lc = {
let mut lockercount = String::new();
io::stdin().read_line(&mut lockercount).expect("expected int");
lockercount.trim().parse().unwrap()
};
Similarly, I would prefer to use enumerate
instead of an explicit counter. An explicit counter might not be incremented by accident, but enumerate
on an iterator will always fit:
println!("Opened lockers:");
for (n, v) in lockers.iter().enumerate() {
if *v == true {
print!("{}, ", n + 1);
}
}
We could also get rid of the if
via filter
, but that's a little bit much:
for (n, _v) in lockers.iter().enumerate().filter(|(_, &b)| b) {
print!("{}, ", n + 1);
}
On the algorithmic part of the code, I'd prefer to use the pos
ition explicitly instead of i*j
:
for i in 1..(lockers.len() + 1) {
let mut pos: usize = i;
while pos <= lc {
lockers[pos - 1] = !lockers[pos - 1];
pos += i;
}
}
Again, instead of explicitly changing a counter, I would prefer an immutable variable, for example via step_by
:
for i in 1..(lockers.len() + 1) {
for pos in (i..lc).step_by(i) {
lockers[pos - 1] = !lockers[pos - 1];
}
}
for i in 1..(lockers.len()+1)
loop. \$\endgroup\$