- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
финальное задание review1 #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добрый вечер, подсказал пару моментов как можно поправить код
| beast::tcp_stream stream_; | ||
| beast::flat_buffer buffer_; | ||
| HttpRequest request_; | ||
| void Read() { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Реализацию всех не шаблонных методов стоит перенести в файл исходных кодов, он для этого и нужен. Если вы будете изменять логику метода в хедере, то вам придется пересобирать все файлы в которых подключен этот хедер. Также можно столкнуться с проблемами взаимной рекурсии, когда у вас в двух хедерах будут функции которые вызывают друг друга.
| 
               | 
          ||
| void Close() { | ||
| beast::error_code ec; | ||
| stream_.socket().shutdown(tcp::socket::shutdown_send, ec); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если вы не обрабатываете код ошибки, то можно использовать перегрузку shutdown без ес. Можно просто вывести в лог сообщение.
| } | ||
| 
               | 
          ||
| // 4.3 Добавляем здания | ||
| if (map_obj.contains("buildings")) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Представьте, что у вас через пару лет игра разрослась и появилось еще 1000 объектов. У каждого объекта добавятся свои свойства и зависимости. В таком случае у вас этот метод разрастется очень сильно и его станет невозможно рефакторить и поддерживать. Стоит декомпозировать загрузки объектов
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пустой файл можно исключить из проекта
| if (target == "/api/v1/maps") { | ||
| HandleGetMapsList(std::move(req), std::forward<Send>(send)); | ||
| return; | ||
| } else if (target.starts_with("/api/v1/maps/")) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эндпоинты я бы законстантил для удобства поддержки кода в будущем
| 
               | 
          ||
| template <typename Body, typename Allocator, typename Send> | ||
| void HandleGetMapsList(http::request<Body, http::basic_fields<Allocator>>&& req, Send&& send) { | ||
| std::string json = "["; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не стоит изобретать велосипед, используйте средства библиотеки boost json для формирования json, это позволит избежать ошибок при его формировании
| } | ||
| json += "],"; | ||
| 
               | 
          ||
| // Buildings | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сериализацию тоже стоит декомпозировать
| file.close(); | ||
| 
               | 
          ||
| // 3. Парсим JSON | ||
| auto value = json::parse(content); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если в JSON будет ошибка, то json::parse выбросит исключение, стоит обернуть в try/catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все верно
| if (road_obj.contains("x1")) { | ||
| int x1 = road_obj.at("x1").as_int64(); | ||
| return model::Road(model::Road::HORIZONTAL, {x0, y0}, x1); | ||
| } else { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если у вас if заканчивается return, то else не обязателен. Можно избежать лишнего ветвления.
if(…){
  code…
  return …
}
code…
Review 1