​​Результаты ревью

Всем большое спасибо за активность, было интересно почитать ваши замечания/вопросы/споры/предложения. Выявились 2 явных лидера по объему найденных недостатков. Так как рассуждения у них были объемные и не строго структурированные, то упомяну обоих. Давайте похлопаем Дмитрию и Михаилу👏👏👏👏

Теперь саммари по пунктам:

❗️ Используется странный стиль глобальные переменные + функции. Много чего можно объединить и/или завернуть в классы.

❗️ Если уж используете глобальные переменные в цпп файле(а это скорее всего он и есть), то используйте пометку static, чтобы не засорять именами скоуп. К функциям это тоже относится.

❗️ Обильное использование сырых указателей на объекты класса. В данном случае это точно code smell и все нужно переделать на умные указатели.

❗️ Бессмысленно использование более сложного гарда std::unique_lock. Обычно он используется в тандеме с кондварами. Здесь будет достаточно std::lock_guard и выделение отдельного скоупа критической секции с помощью фигурных скобок {}.

❗️ Странное завершение цикла при достижении конца очереди. Это накладывает на вызывающий код ответственность за обработку этой ситуации. Конкретно в таком простом варианте очереди на мьютексте логично оставить цикл бесконечным и при достижении конца продолжать цикл заново.

❗️ Странный слип под локом в DequeueJob. Слишком жирно потоку преднамеренно спать с загробастанным мьютексом. Наверное автор просто не подумал, но все же так делать не надо и этот слип нужно вынести из критической секции.

❗️ Странный принт под локом EnqueueJob. Слишком долгая операции и семантически не относящаяся к критической секции. Нужно вынести за пределы лока.

❗️ Странный выбор контейнера для очереди. Если уж так не хочется писать свой класс, то в STL есть прекрасный std::queue, который как бы прям прыгает с экрана монитора и говорит: "Нужна очередь и выбираешь не меня? Больной ублюдок..."

❗️ Так как NewJob - довольно маленькая структура, то передавать ее везде можно по значению, вместо указателей. Ну или сразу использовать std::unique_ptr с заделом на будущее ообогащение задачи.

❗️ Нет graceful завершения обработки очереди. Нужно как-то обрабатывать ситуацию, когда нам захочется закончить разгребать очередь и возможно сделать еще какие-то действия, но мы не хотим это делать через простой head shot приложения.

❗️ Стандартный подход к написанию очередей - использование кондваров. Консюмер спит до тех пор, пока продюсер не уведомит его о наличии новой задачи. Пример можно посмотреть тут.

❗️ Архитектурные решения в этом коде довольно бедные. Понятно, что код простой и не претендует на высокие материи и на место на обложке Cosmopolitan. Но хоть что-то же должно быть. По дизайну видение у всех может быть разное, но тут явно все хромает.

❗️ Уж не буду отдельными пунктами упоминать мелкие странные вещи типа "delete job, job = Null" или конструктор для простой структуры(можно агрегатную инициализацию использовать), юзинг и тд.

Итого довольно внушительный список получился. Конечно, это не продовый код и писал его человек явно в начале своего пути знакомства с многопоточкой и плюсами в целом. Так что не судите строго этого маленького китайского мальчика. Никто не родился вундеркиндом по С++.

"Исправленную версию" можете найти в комментах.

Fix your flaws. Stay cool.