Skip to main content
Code Review

Return to Answer

added 120 characters in body
Source Link
  • Always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

  • In fact, we can go further and remove the &mut keyword for nums2, as nums2 is only read from and not written to.

  • Finally, the variables m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can write pub fn merge(nums1: &mut [i32], mut i1: usize, ... and omit the variables m and n. I would personally also rename these to n1 and n2 for clarity.

pub fn merge(nums1: &mut [i32], mut n1: usize, nums2: &mut [i32]&[i32], mut n2: usize) {
 while n1 > 0 && n2 > 0 {
 if nums1[n1 - 1] > nums2[n2 - 1] {
 n1 -= 1;
 nums1[n1 + n2] = nums1[n1];
 } else {
 n2 -= 1;
 nums1[n1 + n2] = nums1[n2];nums2[n2];
 }
 }
 while n2 > 0 {
 n2 -= 1;
 nums1[n2] = nums2[n2];
 }
}
  • Always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

  • Finally, the variables m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can write pub fn merge(nums1: &mut [i32], mut i1: usize, ... and omit the variables m and n. I would personally also rename these to n1 and n2 for clarity.

pub fn merge(nums1: &mut [i32], mut n1: usize, nums2: &mut [i32], mut n2: usize) {
 while n1 > 0 && n2 > 0 {
 if nums1[n1 - 1] > nums2[n2 - 1] {
 n1 -= 1;
 nums1[n1 + n2] = nums1[n1];
 } else {
 n2 -= 1;
 nums1[n1 + n2] = nums1[n2];
 }
 }
 while n2 > 0 {
 n2 -= 1;
 nums1[n2] = nums2[n2];
 }
}
  • Always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

  • In fact, we can go further and remove the &mut keyword for nums2, as nums2 is only read from and not written to.

  • Finally, the variables m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can write pub fn merge(nums1: &mut [i32], mut i1: usize, ... and omit the variables m and n. I would personally also rename these to n1 and n2 for clarity.

pub fn merge(nums1: &mut [i32], mut n1: usize, nums2: &[i32], mut n2: usize) {
 while n1 > 0 && n2 > 0 {
 if nums1[n1 - 1] > nums2[n2 - 1] {
 n1 -= 1;
 nums1[n1 + n2] = nums1[n1];
 } else {
 n2 -= 1;
 nums1[n1 + n2] = nums2[n2];
 }
 }
 while n2 > 0 {
 n2 -= 1;
 nums1[n2] = nums2[n2];
 }
}
added 77 characters in body
Source Link
  • m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can use:

    Always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

pub fn merge(nums1: &mut [i32], mut i1: usize, nums2: &mut [i32], mut i2: usize) {
  • Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like.

    Finally, the variables m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can write pub fn merge(nums1: &mut [i32], mut i1: usize, ... and omit the variables m and n. I would personally also rename these to n1 and n2 for clarity.

    Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.
pub fn merge(nums1: &mut [i32], mut i1n1: usize, nums2: &mut [i32], mut i2n2: usize) {
 while i1n1 > 0 && i2n2 > 0 {
 if nums1[i1nums1[n1 - 1] > nums2[i2nums2[n2 - 1] {
 i1n1 -= 1;
 nums1[i1nums1[n1 + i2]n2] = nums1[i1];nums1[n1];
 } else {
 i2n2 -= 1;
 nums1[i1nums1[n1 + i2]n2] = nums1[i2];nums1[n2];
 }
 }
 while i2n2 > 0 {
 i2n2 -= 1;
 nums1[i2]nums1[n2] = nums2[i2];nums2[n2];
 }
}
  • m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can use:
pub fn merge(nums1: &mut [i32], mut i1: usize, nums2: &mut [i32], mut i2: usize) {
  • Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.
pub fn merge(nums1: &mut [i32], mut i1: usize, nums2: &mut [i32], mut i2: usize) {
 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
 i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}
  • Always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

  • Finally, the variables m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can write pub fn merge(nums1: &mut [i32], mut i1: usize, ... and omit the variables m and n. I would personally also rename these to n1 and n2 for clarity.

pub fn merge(nums1: &mut [i32], mut n1: usize, nums2: &mut [i32], mut n2: usize) {
 while n1 > 0 && n2 > 0 {
 if nums1[n1 - 1] > nums2[n2 - 1] {
 n1 -= 1;
 nums1[n1 + n2] = nums1[n1];
 } else {
 n2 -= 1;
 nums1[n1 + n2] = nums1[n2];
 }
 }
 while n2 > 0 {
 n2 -= 1;
 nums1[n2] = nums2[n2];
 }
}
deleted 67 characters in body
Source Link

Once you make this change, you can get rid of all those pesky as usize annotations, but now you have a different problem: you are using -1 to indicate when there's no more elements to insert, and -1 isn't a valid usize. Instead, best practice here is to add 1 to all your variables, which also leads to cleaner code. The >=0 checks become >0 checks instead, and the -1 calculations are moved into the while loop. With this change, the beginning of the function becomes:

Our next observation for improvement is that insert_pos is always just equal to i1 + i2. So let's just removereplace all instances of it! We end up withi1 + i2. Once we do this:, you'll notice something fascinating -- your second while loop actually does nothing and can be removed:

pub fn merge(nums1: &mut Vec<i32>, m: usize, nums2: &mut Vec<i32>, n: usize) {
 let mut i1 = m;
 let mut i2 = n;
 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
  i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i1 > 0 {
 i1 -= 1;
 nums1[i1] = nums1[i1];
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}

Hey, wait -- now that we've cleaned up a bit, we can see that that second while loop is completely unnecessary! Notice you're just copying num1[i1] into itself. SoOnce we can entirely remove that loop, and we're left with just the first and third loops.

Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> argumentsBefore we get back to&mut [i32], because the vectors are not being resized at all in the function body, which is correct. Here's our final updated code, let's take a look at a couple of other improvements:

  • m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can use:
pub fn merge(nums1: &mut [i32], mmut i1: usize, nums2: &mut [i32], nmut i2: usize) {
  • Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

Here's our final code, with all of the above changes:

pub fn merge(nums1: &mut let[i32], mut i1 = m;
: usize, nums2: &mut let[i32], mut i2: =usize) n;
{
 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
 i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}

Once you make this change, you can get rid of all those pesky as usize annotations, but now you have a different problem: you are using -1 to indicate when there's no more elements to insert, and - isn't a valid usize. Instead, best practice here is to add 1 to all your variables, which also leads to cleaner code. The >=0 checks become >0 checks instead, and the -1 calculations are moved into the while loop. With this change, the beginning of the function becomes:

Our next observation for improvement is that insert_pos is always just equal to i1 + i2. So let's just remove it! We end up with this:

pub fn merge(nums1: &mut Vec<i32>, m: usize, nums2: &mut Vec<i32>, n: usize) {
 let mut i1 = m;
 let mut i2 = n;
 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
  i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i1 > 0 {
 i1 -= 1;
 nums1[i1] = nums1[i1];
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}

Hey, wait -- now that we've cleaned up a bit, we can see that that second while loop is completely unnecessary! Notice you're just copying num1[i1] into itself. So we can entirely remove that loop, and we're left with just the first and third loops.

Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to&mut [i32], because the vectors are not being resized at all in the function body, which is correct. Here's our final code:

pub fn merge(nums1: &mut [i32], m: usize, nums2: &mut [i32], n: usize) {
 let mut i1 = m;
 let mut i2 = n;

 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
 i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}

Once you make this change, you can get rid of all those pesky as usize annotations, but now you have a different problem: you are using -1 to indicate when there's no more elements to insert, and -1 isn't a valid usize. Instead, best practice here is to add 1 to all your variables. The >=0 checks become >0 checks instead, and the -1 calculations are moved into the while loop. With this change, the beginning of the function becomes:

Our next observation for improvement is that insert_pos is always just equal to i1 + i2. So let's just replace all instances of it withi1 + i2. Once we do this, you'll notice something fascinating -- your second while loop actually does nothing and can be removed:

 while i1 > 0 {
 i1 -= 1;
 nums1[i1] = nums1[i1];
 }

Once we remove that, we're left with just the first and third loops. Before we get back to the final updated code, let's take a look at a couple of other improvements:

  • m and n are immediately assigned to i1 and i2, and otherwise removed. So it would be cleaner to just declare i1 and i2 as mut in the function signature. That is, we can use:
pub fn merge(nums1: &mut [i32], mut i1: usize, nums2: &mut [i32], mut i2: usize) {
  • Finally, always try running cargo clippy! It's an automatic tool which always gives helpful suggestions to make your code more Rust-like. Here, it wants us to convert the &mut Vec<i32> arguments to &mut [i32], because the vectors are not being resized at all in the function body, which is correct.

Here's our final code, with all of the above changes:

pub fn merge(nums1: &mut [i32], mut i1: usize, nums2: &mut [i32], mut i2: usize) {
 while i1 > 0 && i2 > 0 {
 if nums1[i1 - 1] > nums2[i2 - 1] {
 i1 -= 1;
 nums1[i1 + i2] = nums1[i1];
 } else {
 i2 -= 1;
 nums1[i1 + i2] = nums1[i2];
 }
 }
 while i2 > 0 {
 i2 -= 1;
 nums1[i2] = nums2[i2];
 }
}
Source Link
Loading
lang-rust

AltStyle によって変換されたページ (->オリジナル) /