Return to Revisions

Here are some things that may help you improve your program.

Fix the bug

There is a problem with updateListExe that occurs in the test program after removing one element from the list. It access freed memory which is an error. The easiest way to avoid this is to simplify the code to this:

template<typename T>
void updateListExe(const Capacity cap, const T& listExecutableId) {
    for (const auto exeId : listExecutableId) {
        auto it{listExeByCapacity[cap].find(exeId)};
        if (it == listExeByCapacity[cap].end()) {
            addExeFromCap(cap, exeId);
        } else {
            removeExeFromCap(cap, exeId);
        }
    }
}

Understand standard container operators

I would expect that someone calling getFirstAvailable with an invalid cap would be surprised to find out that it modifies the object because the function uses operator[]. That operator inserts the new value if it's not already there, and that is probably not what is intended. I would suggest making this a const function and having it throw if the cap is not found. Here's one way to do that:

static bool isAvailable(const std::pair<const T, Disponibility>& pair) {
    return pair.second == Disponibility::Available;
}

T getFirstExeAvailable(const Capacity cap) const
{
    const auto end{ listExeByCapacity.at(cap).cend()};
    auto it{std::find_if(listExeByCapacity.at(cap).cbegin(), end, isAvailable)};
    return it == end ? ERROR : it->first;
}

This will throw a std::out_of_range error if the passed cap is not in the map.

Use templates consistently

In the code quoted above the executable ID is a templated type, while everywhere else it is coded as an int. Choose one or the other but not both.

Consider another data structure

The application looks like a task prioritization scheme for a scheduler. It may be more appropriate to use a std::priority_queue rather than the current data structure, and it also has logarithmic insertion and retrieval times.

Support the use of a std::initializer_list

The constructor takes a std::vector<Capacity> as the argument, and constructing the object currently looks like this (note I have added the template):

std::vector<Capacity> vectorCapacity = {Capacity::TRT_1, Capacity::TRT_2};
ManageListExe<int> managerList(vectorCapacity);

It seems to me that it would be much nicer to do this instead:

ManageListExe<int> managerList{Capacity::TRT_1, Capacity::TRT_2};

That is easily supported with this constructor:

ManageListExe(std::initializer_list<Capacity> listCap)
{
    for(const auto cap : listCap)
    {
        listExeByCapacity[cap] = {};
    }
}

Use better naming

In the comments it has already been pointed out that "Disponibility" is a rather unusual word for many native English speakers, and "Availability" might be a better word. I would say that ManageListExe could be improved as well. I would suggest Executables.

Write a standard inserter

Instead of the relatively inflexible printListExe, I would suggest writing a standard inserter instead with the signature:

friend std::ostream& operator<<(std::ostream& out, const ManageListExe& el);

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Use const auto & where practical

Use this common C++11 idiom to operate non-destructively on all of the data members of collection:

for (const auto &val : myCollection) {
    std::cout << val << '\n';
}

The intent here is to print the values of all of the collection values without modifying them (hence const) and without copying them (hence &).

Consider using std::optional

If your compiler supports C++17, I would suggest that std::optional would be a better choice of return value for getFirstExeAvailable(). The requirement for an ERROR value would disappear and the code would more naturally express the intent.

Use unordered data structures for performance

Unless you actually need them in order, there is often a performance gain in using, for example, std::unordered_map rather than std::map.