Skip to main content
updated with comment info
Source Link
trent
  • 1.1k
  • 7
  • 15
  • Create test cases so you can have some confidence while refactoring.

  • Don't create empty structs (Solution) just to give them functions. next_permutation is perfectly fine as a free function. If you have to create a bad API to satisfy LeetCode or whatever, write the good API first, and then wrap it. If the API required is so bad you can't wrap a good API with it, it's probably a waste of time.

  • Use rustfmt.

  • Accept &mut [T] instead of &mut Vec<T> when you don't need to change the size.

  • let first = 0;
    let last = nums.len();
    

    Don't give unnecessary symbolic names to things that aren't used symbolically. 0 is far more obviously the index of the first element in a slice than first. Furthermore, you don't even need to provide the bounds of the slice when indexing with [..]; they're implied. So you need these even less than you perhaps thought.

  • i, i1, i2 are meaningless. How about some descriptive names?

  • I prefer to put each loop in a block with its state variables to limit the scope of the mutability.

  • let i1 = i;
    

    Similar to the note on first and last above, don't make variables that can be trivially calculated from other variables. The relationship between i and i + 1 is way more obvious than i and i1.

  • if nums[i] < nums[i + 1] { ... }
    

    Why is this body nested in the outer loop? The algorithm breaks fairly easily into four steps:

    1. Get the index of the rightmost ascending pair of values
    2. Get the index of the rightmost value greater than the first element of that pair
    3. Swap the values at these two indices
    4. Reverse the slice starting just after the first index. You have 2, 3, and 4 nested inside 1, which makes the algorithm look more complicated than it actually is.

    You have 2, 3, and 4 nested inside 1, which makes the algorithm look more complicated than it actually is.

  • let tmp = nums[i];
    nums[i] = nums[i2];
    nums[i2] = tmp;
    

    Use nums.swap(i, i2) instead.

  • As mentioned in the reddit comments, there's an iterator method rposition that finds the last item matching some predicate (if the iterator is one that can be iterated from both ends). You can use this to linearize the first (outer) loop, which may be easier to reason about. Don't go overboard, though: iterators are great for certain tasks and not so great for others. If you feel like the logic and flow of the code is better with a loop, just write the loop.

  • As for the inner loop, since the trailing subslice is by definition monotonically decreasing, you can search for the swap point using binary search. It's not likely to make a big performance difference, though, and the logic might be a little harder to follow, so you might want to use rposition here as well.

AppliedApplied

pub fn next_permutation(nums: &mut [i32]) -> bool {
    if nums.len() < 2use {std::cmp::Ordering;
        return// false;
or use feature(array_windows) on }nightly
    let last_ascending_pairlast_ascending = {
        let mut i =match nums.lenwindows(2) - 1;
        loop {
            i -= 1;

            if.rposition(|w| nums[i]w[0] < nums[i + 1]w[1]) {
                break i;
          Some(i) => }i,
            if i ==None 0=> {
                nums.reverse();
                return false;
            }
        }
    };

    let last_greater_than_nswap_with = {
nums[last_ascending + 1..]
      let mut i.binary_search_by(|n| =i32::cmp(&nums[last_ascending], numsn).lenthen(Ordering::Less);
        loop {
            i -= 1;
            if nums[last_ascending_pair] < nums[i] {
                break i;)
            }
  .unwrap_err(); // cannot fail because the }
binary search will never };succeed
    nums.swap(last_ascending_pairlast_ascending, last_greater_than_nlast_ascending + swap_with);
    nums[last_ascending_pairnums[last_ascending + 1..].reverse();
    true
}

You might think there's a more "iteratory" way to write this, and there might well be. I gave it a shot but I don't love the result. Pick your poison, I guess.

  • Create test cases so you can have some confidence while refactoring.

  • Don't create empty structs (Solution) just to give them functions. next_permutation is perfectly fine as a free function. If you have to create a bad API to satisfy LeetCode or whatever, write the good API first, and then wrap it. If the API required is so bad you can't wrap a good API with it, it's probably a waste of time.

  • Use rustfmt.

  • Accept &mut [T] instead of &mut Vec<T> when you don't need to change the size.

  • let first = 0;
    let last = nums.len();
    

    Don't give unnecessary symbolic names to things that aren't used symbolically. 0 is far more obviously the index of the first element in a slice than first. Furthermore, you don't even need to provide the bounds of the slice when indexing with [..]; they're implied. So you need these even less than you perhaps thought.

  • i, i1, i2 are meaningless. How about some descriptive names?

  • I prefer to put each loop in a block with its state variables to limit the scope of the mutability.

  • let i1 = i;
    

    Similar to the note on first and last above, don't make variables that can be trivially calculated from other variables. The relationship between i and i + 1 is way more obvious than i and i1.

  • if nums[i] < nums[i + 1] { ... }
    

    Why is this body nested in the outer loop? The algorithm breaks fairly easily into four steps:

    1. Get the index of the rightmost ascending pair of values
    2. Get the index of the rightmost value greater than the first element of that pair
    3. Swap the values at these two indices
    4. Reverse the slice starting just after the first index. You have 2, 3, and 4 nested inside 1, which makes the algorithm look more complicated than it actually is.
  • let tmp = nums[i];
    nums[i] = nums[i2];
    nums[i2] = tmp;
    

    Use nums.swap(i, i2) instead.

Applied

pub fn next_permutation(nums: &mut [i32]) -> bool {
    if nums.len() < 2 {
        return false;
    }
    let last_ascending_pair = {
        let mut i = nums.len() - 1;
        loop {
            i -= 1;

            if nums[i] < nums[i + 1] {
                break i;
            }
            if i == 0 {
                nums.reverse();
                return false;
            }
        }
    };

    let last_greater_than_n = {
        let mut i = nums.len();
        loop {
            i -= 1;
            if nums[last_ascending_pair] < nums[i] {
                break i;
            }
        }
    };
    nums.swap(last_ascending_pair, last_greater_than_n);
    nums[last_ascending_pair + 1..].reverse();
    true
}

You might think there's a more "iteratory" way to write this, and there might well be. I gave it a shot but I don't love the result. Pick your poison, I guess.

  • Create test cases so you can have some confidence while refactoring.

  • Don't create empty structs (Solution) just to give them functions. next_permutation is perfectly fine as a free function. If you have to create a bad API to satisfy LeetCode or whatever, write the good API first, and then wrap it. If the API required is so bad you can't wrap a good API with it, it's probably a waste of time.

  • Use rustfmt.

  • Accept &mut [T] instead of &mut Vec<T> when you don't need to change the size.

  • let first = 0;
    let last = nums.len();
    

    Don't give unnecessary symbolic names to things that aren't used symbolically. 0 is far more obviously the index of the first element in a slice than first. Furthermore, you don't even need to provide the bounds of the slice when indexing with [..]; they're implied. So you need these even less than you perhaps thought.

  • i, i1, i2 are meaningless. How about some descriptive names?

  • I prefer to put each loop in a block with its state variables to limit the scope of the mutability.

  • let i1 = i;
    

    Similar to the note on first and last above, don't make variables that can be trivially calculated from other variables. The relationship between i and i + 1 is way more obvious than i and i1.

  • if nums[i] < nums[i + 1] { ... }
    

    Why is this body nested in the outer loop? The algorithm breaks fairly easily into four steps:

    1. Get the index of the rightmost ascending pair of values
    2. Get the index of the rightmost value greater than the first element of that pair
    3. Swap the values at these two indices
    4. Reverse the slice starting just after the first index.

    You have 2, 3, and 4 nested inside 1, which makes the algorithm look more complicated than it actually is.

  • let tmp = nums[i];
    nums[i] = nums[i2];
    nums[i2] = tmp;
    

    Use nums.swap(i, i2) instead.

  • As mentioned in the reddit comments, there's an iterator method rposition that finds the last item matching some predicate (if the iterator is one that can be iterated from both ends). You can use this to linearize the first (outer) loop, which may be easier to reason about. Don't go overboard, though: iterators are great for certain tasks and not so great for others. If you feel like the logic and flow of the code is better with a loop, just write the loop.

  • As for the inner loop, since the trailing subslice is by definition monotonically decreasing, you can search for the swap point using binary search. It's not likely to make a big performance difference, though, and the logic might be a little harder to follow, so you might want to use rposition here as well.

Applied

pub fn next_permutation(nums: &mut [i32]) -> bool {
    use std::cmp::Ordering;
    // or use feature(array_windows) on nightly
    let last_ascending = match nums.windows(2).rposition(|w| w[0] < w[1]) {
        Some(i) => i,
        None => {
            nums.reverse();
            return false;
        }
    };

    let swap_with = nums[last_ascending + 1..]
        .binary_search_by(|n| i32::cmp(&nums[last_ascending], n).then(Ordering::Less))
        .unwrap_err(); // cannot fail because the binary search will never succeed
    nums.swap(last_ascending, last_ascending + swap_with);
    nums[last_ascending + 1..].reverse();
    true
}
Source Link
trent
  • 1.1k
  • 7
  • 15

In no particular order:

  • Create test cases so you can have some confidence while refactoring.

  • Don't create empty structs (Solution) just to give them functions. next_permutation is perfectly fine as a free function. If you have to create a bad API to satisfy LeetCode or whatever, write the good API first, and then wrap it. If the API required is so bad you can't wrap a good API with it, it's probably a waste of time.

  • Use rustfmt.

  • Accept &mut [T] instead of &mut Vec<T> when you don't need to change the size.

  • let first = 0;
    let last = nums.len();
    

    Don't give unnecessary symbolic names to things that aren't used symbolically. 0 is far more obviously the index of the first element in a slice than first. Furthermore, you don't even need to provide the bounds of the slice when indexing with [..]; they're implied. So you need these even less than you perhaps thought.

  • i, i1, i2 are meaningless. How about some descriptive names?

  • I prefer to put each loop in a block with its state variables to limit the scope of the mutability.

  • let i1 = i;
    

    Similar to the note on first and last above, don't make variables that can be trivially calculated from other variables. The relationship between i and i + 1 is way more obvious than i and i1.

  • if nums[i] < nums[i + 1] { ... }
    

    Why is this body nested in the outer loop? The algorithm breaks fairly easily into four steps:

    1. Get the index of the rightmost ascending pair of values
    2. Get the index of the rightmost value greater than the first element of that pair
    3. Swap the values at these two indices
    4. Reverse the slice starting just after the first index. You have 2, 3, and 4 nested inside 1, which makes the algorithm look more complicated than it actually is.
  • let tmp = nums[i];
    nums[i] = nums[i2];
    nums[i2] = tmp;
    

    Use nums.swap(i, i2) instead.

Applied

pub fn next_permutation(nums: &mut [i32]) -> bool {
    if nums.len() < 2 {
        return false;
    }
    let last_ascending_pair = {
        let mut i = nums.len() - 1;
        loop {
            i -= 1;

            if nums[i] < nums[i + 1] {
                break i;
            }
            if i == 0 {
                nums.reverse();
                return false;
            }
        }
    };

    let last_greater_than_n = {
        let mut i = nums.len();
        loop {
            i -= 1;
            if nums[last_ascending_pair] < nums[i] {
                break i;
            }
        }
    };
    nums.swap(last_ascending_pair, last_greater_than_n);
    nums[last_ascending_pair + 1..].reverse();
    true
}

You might think there's a more "iteratory" way to write this, and there might well be. I gave it a shot but I don't love the result. Pick your poison, I guess.