Manage lists of executable id

Question

I need to manage lists of executables Id. Each executable belongs to a capacity (obviously the list of capacities in the code is just an example).

For every capacity we need to be able to find the first executableId which is Available (ie not doing any treatment) and to modify the availability of an executable Id. executable Ids can be seen as the process Id.

Is the idea of using map a good one? Or should I use two vectors for every capacities, one for the executables Availables and the other for the executables In progress?

#include <iostream>
#include <map>
#include <vector>
#include <algorithm>


constexpr int ERROR = -1;

enum class Disponibility {
    Available,
    In_Progress,
};

enum class Capacity {
    TRT_1,
    TRT_2,
    TRT_3,
};

std::ostream& operator<<(std::ostream &os, const Capacity & cap) {
    switch(cap) {
        case Capacity::TRT_1:
            os << "TRT_1";
            break;
        case Capacity::TRT_2:
            os << "TRT_2";
            break;
        case Capacity::TRT_3:
            os << "TRT_3";
        default:
            break;
    }
    
    return os;
}

std::ostream& operator<<(std::ostream &os, const Disponibility & dispo) {
    switch(dispo) {
        case Disponibility::Available:
            os << "Available";
            break;
        case Disponibility::In_Progress:
            os << "In_Progress";
            break;
        default:
            break;
    }
    
    return os;
}




/*
Initialize by passing a vector of all capacities the Manager should deal with
Ideas : - Allow constructor to be done with any iterable container  ?
*/
class ManageListExe {
    public:
        ManageListExe(const std::vector<Capacity> &listCap)
        {
            for(const auto cap : listCap)
            {
                listExeByCapacity[cap] = {};
            }
        }
        
        template<typename T>
        void updateListExe(const Capacity cap, const T& listExecutableId)
        {
            //Add executable Id when not in list
            for(const auto exeId: listExecutableId)
            {
                if(listExeByCapacity[cap].find(exeId) == listExeByCapacity[cap].end())
                {
                    addExeFromCap(cap, exeId);
                }
            }
            
            //Remove from listExeByCapacity executableId which are not anymore in listExecutableId
            for(const auto val: listExeByCapacity[cap])
            {
                if(std::find(listExecutableId.cbegin(), listExecutableId.cend(), val.first) == listExecutableId.cend())
                {
                    removeExeFromCap(cap, val.first);
                }
            }
        }
        
        int getFirstExeAvailable(const Capacity cap)
        {
            //return first exe available from the list
            for(const auto val: listExeByCapacity[cap])
            {
                if(val.second == Disponibility::Available)
                {
                    return val.first;
                }
            }
            return ERROR;
        }
        
        void changeDisponibility(const Capacity cap, const int executableId, const Disponibility dispo)
        {
            listExeByCapacity[cap][executableId] = dispo;
        }
        
        void printListExe()
        {
            for(const auto listByCap:listExeByCapacity)
            {
                for(const auto listExe:listByCap.second)
                {
                    std::cout << "Capacity : " << listByCap.first
                    << ", ExecutableId : " << listExe.first << " : "
                    << listExe.second << "\n";
                }
            }
        }
        
        void printListExeFromCapacity(const Capacity cap)
        {
            std::cout << "Capacity : " << cap << "\n";
            for(const auto listExe:listExeByCapacity[cap])
            {
                std::cout << "ExecutableId : " << listExe.first << " : "
                << listExe.second << "\n";
            }
        }
            
    private:
        
        void addExeFromCap(const Capacity cap, const int executableId)
        {
            listExeByCapacity[cap][executableId] = Disponibility::Available;
        }
        
        void removeExeFromCap(const Capacity cap, const int executableId)
        {
            listExeByCapacity[cap].erase(executableId);
        }
        
        std::map<Capacity, std::map<int, Disponibility>> listExeByCapacity;
                        
};


int main()
{
    std::cout << "Start tests" << std::endl;
    
    //Test creating Manager
    std::vector<Capacity> vectorCapacity = {Capacity::TRT_1, Capacity::TRT_2};
    ManageListExe managerList(vectorCapacity);
    
    //Test updating capacities
    std::vector<int> listExe = {1,2,0};
    managerList.updateListExe(Capacity::TRT_1, listExe);
    
    listExe.push_back(4);
    managerList.updateListExe(Capacity::TRT_1, listExe);
    std::cout << "---\nWe should see executableId 0,1,2,4 from TRT_1 : " << std::endl;
    managerList.printListExeFromCapacity(Capacity::TRT_1);
    
    //Test with 2 capacities
    managerList.updateListExe(Capacity::TRT_2, listExe);
    std::cout << "---\nWe should see executableId 0,1,2,4 from TRT_1 and TRT_2 : " << std::endl;
    managerList.printListExe();
    
    //Test changing one disponibility
    managerList.changeDisponibility(Capacity::TRT_1, 0, Disponibility::In_Progress);
    std::cout << "---\nWe should see executableId 0,1,2,4 from TRT_1 and executable 0 should be in progress : " << std::endl;
    managerList.printListExeFromCapacity(Capacity::TRT_1);
    
    //Test remove one element in list
    listExe.pop_back();
    managerList.updateListExe(Capacity::TRT_1, listExe);
    std::cout << "---\nWe should see executableId 0,1,2 from TRT_1 and 0,1,2,4 from TRT_2 : " << std::endl;
    managerList.printListExe();
    
    //Test Getting first executableId Available
    int firstExeAvailable = managerList.getFirstExeAvailable(Capacity::TRT_1);
    std::cout << "First executableId available from capacity TRT_1 (should be 1) : " << firstExeAvailable << std::endl;
    
    return 0;
    
}

Answer 1

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 accesses 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.