Feedback on how Improve and be more memory efficient

code review
1

Hi everyone,

i very very recently decided to learn rust, i'm mainly a high level programmer so it's very confusing and difficult working with rust especially with low level memory management and the borrow checker.

after countless errors i managed to write this code, it's just a sorter i made with no previous experience with sorting algorithms and it works!
if anyone has any feedback on how to improve and make this wacky code be more memory efficient that would be great!

fn main() {
    let vec1: Vec<i32> = vec![5, 6, 0, 4, 88, 2, 4, 9, 9, 54, 32, 1, 51, 7];
    let sorted_vec: Vec<i32> = sort_vec(&vec1);
    for num in &sorted_vec {
        println!("{}", num);
    }
}

fn sort_vec(vector: &[i32]) -> Vec<i32> {
    let mut sorted: Vec<i32> = vector.to_vec();
    let mut temp: i32;
    let mut curnum: i32;
    let mut num: i32;
    for curidx in 0..sorted.len() {
        for idx in 0..sorted.len() {
            num = sorted[idx];
            curnum = sorted[curidx];
            if curidx == idx {
                continue;
            }
            if curnum < num && curidx > idx {
                temp = curnum;
                sorted[curidx] = num;
                sorted[idx] = temp;
            }
        }
    }
    sorted
}
1 Like
2

It seems like the best way to improve your code would be to use a better sorting algorithm.

Note that Rust comes with methods for swapping:

sorted.swap(curidx, idx);
2 Likes
3

You could make your second loop run from 0..curidx.
This way you can throw away two of the conditions in the loop body, because they're either never or always fulfilled.
That doesn't help with memory efficiency though.

4

It would be more idiomatic to make sort_vec work with a mutable reference. This avoids cloning the original vector if you don't need to keep the original.

If you want to keep the original, I would clone it outside the sort_vec function:

fn main() {
    let vec1: Vec<i32> = vec![5, 6, 0, 4, 88, 2, 4, 9, 9, 54, 32, 1, 51, 7];
    let mut sorted_vec = vec1.clone(); // assuming you want to keep the unsorted original
    sort_vec(&mut sorted_vec);
    for num in &sorted_vec {
        println!("{}", num);
    }
}

fn sort_vec(vector: &mut [i32]) {
    let mut temp: i32;
    let mut curnum: i32;
    let mut num: i32;
    for curidx in 0..vector.len() {
        for idx in 0..vector.len() {
            num = vector[idx];
            curnum = vector[curidx];
            if curidx == idx {
                continue;
            }
            if curnum < num && curidx > idx {
                temp = curnum;
                vector[curidx] = num;
                vector[idx] = temp;
            }
        }
    }
}

(Playground)

Note that slice::sort from the standard library also works on a &mut reference.

Yes, the sorting algorithm itself is not efficient. idx could start with curidx+1 instead of starting with 0:

     for curidx in 0..vector.len() {
-        for idx in 0..vector.len() {
+        for idx in curidx + 1..vector.len() {

And also, curidx doesn't need to run to vector.len() but only to vector.len()-1:

-     for curidx in 0..vector.len() {
+     for curidx in 0..vector.len() - 1 {
         for idx in curidx + 1..vector.len() {

Then you could remove all this:

-            if curidx == idx {
-                continue;
-            }
-            if curnum < num && curidx > idx {
+            if curnum > num {
                 temp = curnum;
                 vector[curidx] = num;
                 vector[idx] = temp;
             }

Moreover, temp isn't needed, because curnum and num are copies already:

-                temp = curnum;
                 vector[curidx] = num;
-                vector[idx] = temp;
+                vector[idx] = curnum;

You don't need curnum and num to be mutable, instead you can just declare them with a more limited scope:

 fn sort_vec(vector: &mut [i32]) {
-    let mut temp: i32;
-    let mut curnum: i32;
-    let mut num: i32;
     for curidx in 0..vector.len() - 1 {
         for idx in curidx + 1..vector.len() {
-            num = vector[idx];
+            let num = vector[idx];
-            curnum = vector[curidx];
+            let curnum = vector[curidx];
             if curnum > num {
                 vector[curidx] = num;
                 vector[idx] = curnum;
             }
         }
     }
 }

That said, @alice pointed out already, that Rust provides a function for swapping (which you could also use): slice::swap.

Here is a cleaned up version, which doesn't use the methods from std:

fn sort_vec(vector: &mut [i32]) {
    for curidx in 0..vector.len() - 1 {
        for idx in curidx + 1..vector.len() {
            let num = vector[idx];
            let curnum = vector[curidx];
            if curnum > num {
                vector[curidx] = num;
                vector[idx] = curnum;
            }
        }
    }
}

(Playground)

But even that algorithm is still not very efficient. There are better sorting algorithms, see Wikipedia for some examples.

Yeah, or this way. In my example curidx is smaller than idx and I changed the comparison.

1 Like
5

well that's really convenient

6

Thank you for your help! i learned a lot about rust and how to write better code from analyzing your improvements, i got really confused when i started learning about memory management, heap and stack and the borrow checker, 80% of debugging i did was just me fighting with the borrow checker.
i still have a long way to go, thank you!

7

Rust is not easy to learn (in my opinion), and it took me a long time to feel (relatively) secure with writing code in Rust. Asking on this forum helped me a lot on my way.

When you are stuck, feel free to ask questions here. It does help to hear about other people's opinion on how to write certain things in the best way (often, Rust provides several ways to achieve the same, and some of the ways may be better than others).

Regarding passing arguments, you can

  • pass them as value (i.e. the function consumes the value),
  • pass a shared reference,
  • pass a mutable (i.e. exclusive) reference.

I was often not sure when to pass a value and when to use references. Nowadays, I try to think on what is most convenient for the called function.

Your sorting algorithm is most efficient when it re-uses the existing memory. Thus making it take a &mut […] argument is a natural choice.

When you want to store something somewhere, taking a Vec<…> may be the better choice.

But I'm still sometimes unsure about which approach is the best for a particular problem. Sometimes there are several ways, and sometimes even a simpler syntax can be a reason to use a slightly less-efficient way (even if that ideally should not happen, but being always most efficient can sometimes lead to complex source code).

2 Likes
8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.