Server-network by Roman-Koshelev · Pull Request #3 · TeamInTeam/Labyrinth

Roman-Koshelev · 2017-12-21T09:07:50Z

Сетевая часть клиента и сервера

leshiy1295

Очень сложно воспринимаемый код из-за большого количества недостатков:

сильная связность - классы недостаточно декомпозированы
недостаточное разбиение по файлам - несколько разных по зоне ответственности классов определяются в одном файле, их методы описываются прямо в определении классов...
использование шаблонных классов наравне с объектным подходом - как минимум, их нужно разделить по классам и файлам
нарушения по стилю и непонятные названия методов и переменных
не всегда удачно выбранные структуры данных - скорее всего tuple от пар будет использовать неудобно разработчикам, которые впоследствии могли бы поддерживать Вашу систему
большое количество копипасты логики в инфраструктурных классах из-за неграмотной декомпозиции архитектуры на уровни и на отдельные классы.
Видно, что много чего умеете и знаете с точки зрения языковых возможностей языка. Но из-за нехватки времени и недостаточной проработки архитектуры код получился сложно поддерживаемым - я не уверен, что Ваши коллеги смогут без проблем с ним разобраться.
Так как времени до защиты осталось мало, а поправить всё до неё Вы, очевидно, уже не успеете, но тем не менее и мне и Вам хочется, чтобы Ваша команда выступила, то сейчас постарайтесь произвести интеграцию с другими участниками Вашей команды (я уже видел, что Вы что-то заливали в master и уже занимаетесь этим), а для повышения итогового количества баллов Вы сможете доработать проект позднее - вплоть до пересдачи в конце января.

leshiy1295 · 2017-12-21T09:09:21Z

+#include <boost/asio.hpp>
+#include <boost/variant.hpp>
+
+#include "../server/game-mechanics/proto/proto.h"


по возможности стоит избегать таких косвенных путей
лучше либо декомпозировать на уровне файлов так, чтобы избегать подобных ситуаций, или подключать директории для поиска через флажок -I

Пока делаем так, пока нет финальной версии и тд

Я понимаю, что это рабочая версия, поэтому и оставляю Вам комментарии и даю возможность поправить. Поэтому лучше либо поправить замечания, либо задать вопрос/объяснить свою точку зрения, почему моё замечание не верно.

leshiy1295 · 2017-12-21T09:10:44Z

+#include <vector>
+#include <cstdlib>
+
+const std::size_t MAX_USERS = 1; // ��� ��� ��������� ��� ����


вроде бы у Вашего товарища по команде есть БД - я так понимаю, что пока что это временная заглушка вместо её использования...

да это так

leshiy1295 · 2017-12-21T09:11:26Z

+
+		const tcp::endpoint endpoint(tcp::v4(), 2000);
+		labyrinth_server server(io_service, endpoint);
+		log("server: [0x0005] Старт сервера");


а что за магические константы в логах?
лучше было бы выделить отдельный класс, который error_code инкапсулировал в себе, как и error_message, а на стороне клиента нужно было лишь вызывать его методы

было наспех расставлено для отладки. тк логи не основная функция продукта, то не заморачивались

Основная задача нашего курса - не сделать наспех, а сделать качественно.
Если говорить про логирование и про продукт, то, возможно, в Вашем конкретно продукте логирование и не является важной его частью, однако сегодня побеждает та команда, у которой лучше налажены процессы, поэтому логирование сегодня - это одно из важнейших требований к любым приложениям.
Мы Вас готовим на системных архитекторов, а не на "тяп-ляп и в продакшн". Поэтому и требования соответствующие

leshiy1295 · 2017-12-21T09:11:46Z

+	try {
+		boost::asio::io_service io_service;
+
+		const tcp::endpoint endpoint(tcp::v4(), 2000);


порты и подобные вещи стоит выносить в конфигурационные файлы

да конечно хорошо иметь конфиг xml и тд. Но нам щас это не важно

Про конфигурацию рассказывалось на лекциях - в тех примерах добавляется она буквально в 7 строк.
Остальные команды, которые планомерно проходили курс, успешно реализуют у себя работу с конфигурационными файлами.
Я уже говорил - поправить всё до защиты Вы уже всё не успеете, но на будущее - для повышение количества баллов - сделать стоит. Это добавляется просто, но принципиально важно с точки зрения архитектуры работы проекта.

leshiy1295 · 2017-12-21T09:15:33Z

+	void do_write(const labyrinth_message& msg) {
+		client_.do_async_write(msg, [this](bool error) {
+			if(!error) {
+				//...


что должно тут происходить?

ничего не должно.

значит эта ветка и не нужна вовсе. Это затрудняет чтение кода и не эффективно с точки зрения процессора

leshiy1295 · 2017-12-21T09:55:00Z

+	std::uint32_t sizeStr = str.size() + 1;
+	memcpy_fl(mem + ch, &sizeStr, sizeof(std::uint32_t), fl);
+	ch += sizeof(std::uint32_t);
+	if (fl) {


семантика этой переменной не ясна, во многом из-за её названия

leshiy1295 · 2017-12-21T09:55:36Z

+struct Start_room
+{
+	Start_room() = default;
+	Start_room(const uint32_t count_players, const char* room_name)


почему тут используется char *, а не string?

leshiy1295 · 2017-12-21T09:57:07Z

+REGISTER_TYPE_1(get_list_room, 14)
+REGISTER_TYPE_1(list_room_package, 15)
+
+inline void memcpy_fl(void * Lhs, void* Rhs, const std::size_t size, const bool fl) {


плохое название метода
тут больше по смыслу подходят слово direction, а не fl...

leshiy1295 · 2017-12-21T09:58:37Z

+	code_vec_pair_int_str(mem, data.vec, false);
+}
+
+inline void  code(char* mem, ShowOtherTurn& data, bool fl) {


откуда взята эта структура?
и почему в ней так много полей?
а также - почему Вы завязываетесь на всех их тут?
я правильно понимаю, что при добавлении очередного поля его нужно будет добавить в том классе, а также ещё и тут?

Структура предоставлена разработчиком логики сервера (я просто пересылаю). Да при добавлении поля в классе придется добавить его тут. Это же функции сериализации, в плюсах рефлексия не очень

это не гибко и вызывает дублирование кода
Вы не просто пересылаете, а Вы завязываетесь на внутреннюю структуру пересылаемого объекта. Одно дело, если было бы некоторое ограниченное количество полей для всех данных - например, header и body. И Вы бы брали всегда для любых типов именно эти два поля и передавали. Тут же отдельная сущность должна по какой-то причине знать о внутренней структуре передаваемых ей сущностей. Это не её зона ответственности.

leshiy1295 · 2017-12-21T10:00:30Z

+
+inline void  code(char* mem, ShowTurn& data, bool fl) {
+	std::size_t ch = 0;
+	memcpy_fl(mem + ch, &data.error, sizeof(data.error), fl);


я всё ещё считаю, что стоило сделать общий класс типа Serializable с этим методом serialize или code, унаследовать все Ваши сообщения также и от этого класса и на стороне описания этого класса уже его реализовать.

не универсально и масштабируемо. Щас класс сообщения может легко использовать кто угодно (не залазя в его код)..

Там также его можно будет использовать. Просто Вы можете использовать обёртку над этим классом, которая уже Serializable. Более того, в Java сериализация модельных объектов - это обычное дело - Вы с этим столкнётесь уже в следующем семестре...

Roman-Koshelev and others added 6 commits December 9, 2017 12:24

Add some server classes

ed11179

Delete cl.cpp

647f7bd

Delete game_map_scheme.h

5480413

Delete labyrint_message.hpp

17896b8

Delete ser.cpp

25effdf

Add files via upload

3bec6ce

Roman-Koshelev requested review from kupihleba and leshiy1295 December 21, 2017 09:07

leshiy1295 requested changes Dec 21, 2017

View reviewed changes

Conversation

Roman-Koshelev commented Dec 21, 2017

Uh oh!

leshiy1295 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants