Is there a better way to keep ids in a vector sequential in C++?

I have a simple C++ program that acts as a todo list, it contains a vector that holds class type of Item to track each todo and their ID.

Is there a more efficient way to make sure that the list of IDs are sequential even after removing an element?

I'm looking for an answer that is objectively more efficient while still following these constraints of being based on vector, using class Item, and keeping track of IDs. Note that this isn't a question on why are you setting IDs in the first place, I want to know what would be the best method while following the constraints.

Example:

Imagine we have an initialized list called todos that is an instance of class Todo

Todo todos = Todo();

todos.add("Take out the trash.");
todos.add("Do the laundry.");
todos.add("Read a book.");
todos.add("Make dinner.");

Our list looks like this when we call list():

{Todo[{Item(id=0, desc="Take out the trash.")}, {Item(id=1, desc="Do the laundry.")}, {Item(id=2, desc="Read a book.")}, {Item(id=3, desc="Make dinner.")}]}

IDs: 0, 1, 2, 3

If I then remove the element at ID 2:

todos.remove(2);

The result would be:

{Todo[{Item(id=0, desc="Take out the trash.")}, {Item(id=1, desc="Do the laundry.")}, {Item(id=3, desc="Make dinner.")}]}

IDs: 0, 1, 3

Now we have removed the element at ID 2, but, I want to keep the IDs sequential. I'm looking for 0, 1, 2.

If I call my fix() function, it will iterate through each index of our todos, check if the elements ID matches the expected next ID, if it doesn't, set the elements ID to the current iteration index (i)

void Todo::fix() {
    for (int i = 0; i < static_cast<int>(todos.size()); i++) {
        if (todos[i].getId() != i) {
            todos[i].setId(i);
        }
    }
}

Here is my code:

todos.add("Take out the trash.");
todos.add("Do the laundry.");
todos.add("Read a book.");
todos.add("Make dinner.");
todos.remove(2);
todos.fix();

If we run this, our todos will now be:

{Todo[{Item(id=0, desc="Take out the trash.")}, {Item(id=1, desc="Do the laundry.")}, {Item(id=2, desc="Make dinner.")}]}

IDs: 0, 1, 2

Great! I got the result I wanted, but here is the question:

Is this the most efficient way to do this task in C++? It seems that if this were a bigger list, it could become resource-intensive and slow. Is there any faster/more efficient way to do this?

Here is the code:

#include <iostream>
#include <vector>

class Item {
    private:
        int id;
        std::string description;
    public:
        Item() : id(0), description("") {}
        Item(int id, std::string description) : id(id), description(description) {}
        int getId() const { return id; }
        void setId(int value) { id = value; }
        std::string getDescription() const { return description; }
        void setDescription(std::string value) { description = value; }
};

class Todo {
    private:
        std::vector<Item> todos;
    public:
        Todo() = default;
        Todo(std::vector<Item> todos) : todos(todos) {};
        bool add(std::string description);
        bool remove(int id);
        bool edit(int id, std::string description);
        void list();
        void fix();
};

bool Todo::add(std::string description) {
    if (description.empty()) {
        return false;
    }

    todos.emplace_back(todos.size(), description);
    return true;
}

bool Todo::remove(int id) {
    if (id < 0 || id >= static_cast<int>(todos.size())) {
        return false;
    }

    todos.erase(todos.begin() + id);
    return true;
}

bool Todo::edit(int id, std::string description) {
    if (id < 0 || id >= static_cast<int>(todos.size()) || description.empty()) {
        return false;
    }

    todos[id].setDescription(description);
    return true;
}

void Todo::list() {
    std::cout << "{Todo[";
    for (size_t i = 0; i < todos.size(); ++i) {
        const auto& item = todos[i];
        std::cout << "{Item(id=" << item.getId() << ", desc=\"" << item.getDescription() << "\")}";
        if (i != todos.size() - 1) std::cout << ", ";
    }
    std::cout << "]}" << std::endl;
}

void Todo::fix() {
    for (int i = 0; i < static_cast<int>(todos.size()); i++) {
        if (todos[i].getId() != i) {
            todos[i].setId(i);
        }
    }
}

int main() {
    Todo todos = Todo();
    todos.add("Take out the trash.");
    todos.add("Do the laundry.");
    todos.add("Read a book.");
    todos.add("Make dinner.");

    todos.remove(2);

    todos.fix();

    todos.list();
    return 0;
}

Solution

I agree with Christophe that you are using the wrong container for what it seems you want to do - but - since you're asking about how to keep using a std::vector<Item> but make it more efficient, here are some ways:

Don't copy when you can move:

Item(int id, std::string description)
    : id(id), description(std::move(description)) {}

Todo(std::vector<Item> todos) : todos(std::move(todos)) {};

Don't make fix public - It invites misuse.
Make fix take an iterator where to start renumbering to not have to iterate over those that don't need renumbering.

Remove the if in fix. All, starting with the supplied iterator, will need renumbering:

class Todo {
private:
    //...
    void fix(std::vector<Item>::iterator it) {
        for (std::size_t idx = std::distance(todos.begin(), it);
            idx != todos.size();
            ++idx)
        {
            todos[idx].setId(idx); // no need for an `if`
        }
    }
public:
    //...
    bool remove(int id) {
        if (id < 0 || id >= static_cast<int>(todos.size())) {
            return false;
        }
        // pass the iterator of the first `Item` to renumber to `fix`:
        fix(todos.erase(todos.begin() + id));
        return true;
    }
};

Make list const-qualified.
Use range-based for loops when you can.

Don't stream directly to std::cout in list and don't use std::endl. Both are inefficient to a certain degree. It's usually more efficient to build a std::string and use \n.

class Todo {
public:
    void list(std::ostream& os = std::cout) const {
        auto& empty = "";
        auto& comma = ", ";
        auto* sep = empty;
        std::string out = "{Todo[";
        for (auto& item : todos) { // range-based for loop
            out += sep;
            sep = comma;
            out += "{Item(id=" + std::to_string(item.getId()) +
                   ", desc=\"" + item.getDescription() + "\")}";
        }
        out += "]}\n";
        os << out;     // only one "expensive" write
    }
};

Remove the check for empty() in the edit condition. It will most probably be optimized away (since it can never be evaluated) and removing it will therefore not make it more efficient - but it confuses readers of your code, thereby making understanding your code less efficient.
Don't use int for id since size(), that you use to populate id, is usually of a larger unsigned type, std::size_t. If you stick with int, before adding a new Item, check if size() > std::numeric_limits<int>::max() first. Changing the id type to std::size_t or checking if size() is larger than an int can hold will not make it more efficient, but may prevent bugs on platforms where the size of an int is as small as 2 octets.