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 thanfirst
. 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
andlast
above, don't make variables that can be trivially calculated from other variables. The relationship betweeni
andi + 1
is way more obvious thani
andi1
. -
if nums[i] < nums[i + 1] { ... }
Why is this body nested in the outer loop? The algorithm breaks fairly easily into four steps:
- Get the index of the rightmost ascending pair of values
- Get the index of the rightmost value greater than the first element of that pair
- Swap the values at these two indices
- 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 aloop
, just write theloop
.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.