Два сеньора C++ - Владимир и Денис - отныне ваши гиды в этом дремучем мире плюсов.
По всем вопросам (+ реклама) @ninjatelegramm
Менеджер: @Spiral_Yuri
Реклама: https://telega.in/c/grokaemcpp
Мы на TGstat: https://tgstat.ru/channel/@grokaemcpp/stat
Post #403
5.19K
Результаты ревью
Всем большое спасибо за активность, было интересно почитать ваши замечания/вопросы/споры/предложения. Выявились 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.
Всем большое спасибо за активность, было интересно почитать ваши замечания/вопросы/споры/предложения. Выявились 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.
- 👍 21
- 👏 9
- ❤ 5
- ❤🔥 2



