Idiomatic append operation

Question

I'm writing a function that will transfer the contents from one Vec to another.

I managed to write two different versions of the same code. One is cleaner, but is potentially slower.

Version 1:

fn move_values<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    loop {
        let value = buffer.pop();
        if value.is_none() {
            return;
        }
        recipient.push(value.unwrap());
    }
}

Version 2:

fn move_values<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    for value in buffer.iter() {
        recipient.push(value.clone());
    }

    buffer.clear();
}

My initial gut feeling is that Version 1 is faster because it only requires a single run through the buffer; while Version 2 is more "Rusty" because it involves iterating over a collection rather than using loop.

Which of these is more idiomatic or "better practice" in general?

Note, I'm aware of append, I'm trying to do this by hand for educational purposes.

Answer 1

Neither. There's a built-in operation for this, Vec::append:

Moves all the elements of other into Self, leaving other empty.

fn move_values<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    recipient.append(buffer);
}

---

Neither of your functions even compile:

fn move_values_1<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    loop {
        let value = buffer.pop();
        if value.is_none() {
            return;
        }
        recipient.push_front(card.unwrap());
    }
}

error[E0425]: unresolved name `card`
 --> src/main.rs:7:30
  |
7 |         recipient.push_front(card.unwrap());
  |                              ^^^^ unresolved name

fn move_values_2<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    for value in buffer.iter() {
        recipient.push_front(value.clone());
    }

    buffer.clear();
}

error: no method named `push_front` found for type `&mut std::vec::Vec<T>` in the current scope
 --> src/main.rs:7:19
  |
7 |         recipient.push_front(card.unwrap());
  |                   ^^^^^^^^^^

---

if I were to implement it myself

Well, there's a reason that it's implemented for you, but sure... let's dig in.

Checking if something is_some or is_none can usually be avoided by pattern matching. For example:

fn move_values_1<T>(buffer: &mut Vec<T>, recipient: &mut Vec<T>) {
    while let Some(v) = buffer.pop() {
        recipient.push(v);
    }
}

Of course, this moves everything in reverse order because pushing and popping to a Vec both occur at the end.

Calling clone doesn't do what you want unless your trait bounds say that T implements Clone. Otherwise, you are just cloning the reference itself.

You can avoid the need for cloning if you drain the values from one collection and insert them into the other:

for value in buffer.drain(..) {
    recipient.push(value);
}

But that for loop is silly, just extend the collection using the iterator:

recipient.extend(buffer.drain(..));

I'd still use the built in append method to do this when transferring between collections of the same type, as it is probably optimized for the precise data layout, and potentially specialized for certain types of data.