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
{
return std::find_if(listExeByCapacity.at(cap).cbegin(),
listExeByCapacity.at(cap).cend(),
isAvailable)->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
.