3
\$\begingroup\$

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;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Since you've mentioned Number Theory, I hope you know that the answer is readily available without for i in 1..(lockers.len()+1) loop. \$\endgroup\$
    – vnp
    May 22, 2021 at 18:50

1 Answer 1

4
\$\begingroup\$

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 position 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];
        }
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.