Client #3

ghost · 2020-11-12T14:15:26Z

Кирилл Верховский
АПО-12
Телеграмм: @Kirillazz
Клиентская часть взаимодействует с пользователем, получает от него команды, отправляет их на сервер. Потом получает ответ и отображает результат пользователю, обновляя данные.

leshiy1295

Продолжайте работу - ещё многое предстоит сделать, а времени - не так много

leshiy1295 · 2020-11-29T23:28:03Z

client/include/application.hpp

+class UserInfo{
+public:
+    UserInfo();
+    UserInfo(std::string& nm, std::string& psw);


посмотрите доклад Джосаттиса про строчки

leshiy1295 · 2020-11-29T23:29:07Z

client/include/application.hpp

+    std::vector<std::string> tags;
+    std::vector<std::string> snapshotPaths;


использовать вектор в интерфейсе - плохая идея, т.к. при необходимости изменения придётся менять весь вызывающий код
инкапсулируйте и используйте PIMPL

leshiy1295 · 2020-11-29T23:29:14Z

client/include/application.hpp

+    std::vector<std::string> snapshotPaths;
+};
+
+


почему всё в одном файле?

leshiy1295 · 2020-11-29T23:29:35Z

client/include/request.hpp

+class Request {
+public:
+    virtual ReqExitStatus buildRequestFromInput(std::istream& s) = 0;
+    virtual std::string RequestToString() = 0;


виртуальный деструктор?

leshiy1295 · 2020-11-29T23:30:07Z

client/include/request.hpp

+    requestPtr _LogInReq;
+    requestPtr _LogOutReq;
+    requestPtr _CreateRoomReq;
+    requestPtr _RemoveRoomReq;
+    requestPtr _AddLinkReq;
+    requestPtr _RemoveLinkReq;
+    requestPtr _LinkReq;
+    requestPtr _ArchiveLinkReq;


переделайте через цепочку ответственностей - рассматривался паттерн на семинаре по тестированию

leshiy1295 · 2020-11-29T23:30:59Z

client/src/application.cpp

+Room::Room(std::string& roomName, std::string& roomId) {}
+void Room::addLink(std::string& newLink) {}
+void Room::removeLink(std::string& linkName) {}
+void Room::addParticipant(std::string& newPart) {}


хотелось бы хотя бы тестовую реализацию видеть...
в идеале - вызовы на уровне интерфейсов

leshiy1295 · 2020-11-29T23:31:51Z

client/test/response.cpp

+    RemoveRoomReq req;
+    RemoveRoomResp resp;
+
+    ASSERT_EQ(resp.doLogic(req, app), RESP_SUCCESS);


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

ghost · 2020-12-10T14:54:52Z

Что использовал из STL:
vector - так как нужен был контейнер, который позволит избежать утечек памяти и облегчить работу с элементами.
vector::iterator - чтобы итерироваться по вектору.
shared_ptr - для использования указателей на реализицию в некоторых классах, а так же хранения объектов базовых классов.
string - для удобной работы со строками.

leshiy1295

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

leshiy1295 · 2020-12-13T19:18:06Z

.github/workflows/blank.yml

+      #- name: test socket
+      #  run: |
+      #    cd client/build
+      #    valgrind --leak-check=full ./test/socket_tests
+
+      #- name: test response
+      #  run: |
+      #    cd client/build
+      #    valgrind --leak-check=full ./test/response_tests
+
+      #- name: test request
+      #  run: |
+      #    cd client/build
+      #    valgrind --leak-check=full ./test/request_tests
+
+      #- name: test client
+      #  run: |
+      #    cd client/build
+      #    valgrind --leak-check=full ./test/client_tests
+
+      #- name: test application
+      #  run: |
+      #    cd client/build
+      #    valgrind --leak-check=full ./test/application_tests


обычно CI запускает один скрипт, а какие тесты запускать/не запускать задаются конфигом и/или уже на уровне этого скрипта

leshiy1295 · 2020-12-13T19:18:45Z

client/include/client.hpp

+#include <string>
+
+#include "socket.hpp"
+#include "socket.hpp"


между первой и второй? :)

leshiy1295 · 2020-12-13T19:18:58Z

client/include/client.hpp

+
+class Client {
+public:
+    Client(const std::string& _host, int _port);


уберите _

leshiy1295 · 2020-12-13T19:20:01Z

client/include/client.hpp

+    Client(const std::string& _host, int _port);
+    ~Client() {}
+    void Connect();
+    void writeToServer(std::string& req);


Request - это куда более сложная структура, чем string
да и у Ваших коллег уже есть определённые структуры - согласуйте их с ними
если это GUI клиент, то непонятно, почему называется writeToServer...
интерфейс обычно работает с логикой экранов/кнопок и пр. Если у некоторых согласно поведению есть необходимость сходить за данными - они обращаются в презентер. Если у него нет состояния - он уже идёт в модель. Там может быть несколько слоёв. Например, кэш, локальная БД и если нигде уже нет - идёт на сервер. При переходе с уровня на уровень всегда можно оставить (и обычно так и делается) коллбэк, который сетит на все промежуточные слои пришедшие данные и возвращают их клиенту через публичный интерфейс

leshiy1295 · 2020-12-13T19:22:40Z

client/include/link.hpp

+    Link(std::string& name, std::string& url);
+    ~Link();
+    const std::string GetLinkName();
+    void addSnapshot(std::string& path);


если есть add, наверное, должен быть и remove?
но из интерфейса непонятно, а куда вообще снэпшот добавляется сейчас...

leshiy1295 · 2020-12-13T19:59:44Z

client/src/userinfo.cpp

@@ -0,0 +1,6 @@
+#include "userinfo.hpp"
+
+UserInfo::UserInfo() : name(""), password("") {}


в дефолтные значения второго конструктора

leshiy1295 · 2020-12-13T19:59:59Z

client/src/view.cpp

+    switch (key) {
+    case 1: {
+        inputStr += "1,";
+        std::cout << "Write name of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr;
+        std::cout << "Write host of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Host*" + appendStr;
+    }
+        break;
+    case 2: {
+        inputStr += "2,";
+        std::cout << "Write name of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr;
+        std::cout << "Write host of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Host*" + appendStr;
+    }
+        break;
+    case 3: {
+        inputStr += "3,";
+        /* std::cout << "Write name of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr + ",";
+        std::cout << "Write host of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Host*" + appendStr + "*Users*"; */
+        inputStr += "*Users*";
+        std::cout << "Write users" << std::endl;
+        while(!appendStr.empty()) {
+            std::cin >> appendStr;
+            inputStr += appendStr + ",";
+        }
+    }
+        break;
+    case 4: {
+        inputStr += "4,";
+        /* std::cout << "Write name of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr + ",";
+        std::cout << "Write host of room" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Host*" + appendStr + "*Users*"; */
+        inputStr += "*Users*";
+        std::cout << "Write users" << std::endl;
+        while(!appendStr.empty()) {
+            std::cin >> appendStr;
+            inputStr += appendStr + ",";
+        }
+    }
+        break;
+    case 5: {
+        inputStr += "5,";
+        std::cout << "Write name of link" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr + ",";
+        std::cout << "Write url of link" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Url*" + appendStr;
+        break;
+    }
+    case 6: {
+        inputStr += "6,";
+        std::cout << "Write name of link" << std::endl;
+        inputStr += "*Name*";
+        std::cin >> inputStr;
+    }
+    case 7: {
+        inputStr += "7,";
+        std::cout << "Write name of link" << std::endl;
+        std::cin >> appendStr;
+        inputStr += "*Name*" + appendStr;
+    }
+        break;
+    case 0:
+        break;
+    default:
+        std::cout << "Wrong command. Try again" << std::endl;
+        break;
+    }


следствие жути сщ switch/case...

leshiy1295 · 2020-12-13T20:00:22Z

client/src/view.cpp

+        std::cout << "Write name of link" << std::endl;
+        std::cin >> appendStr;


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

leshiy1295 · 2020-12-13T20:01:12Z

client/src/view.cpp

+    std::cout << "- 1.Create Room" << std::endl
+              << "- 2.Remove Room" << std::endl
+              << "- 3.Add users" << std::endl
+              << "- 4.Remove users" << std::endl


при добавлении новой команды в середину сломается вся программа
нужно генерировать описание из модели
возможно, с помощью XX Macro...

leshiy1295 · 2020-12-13T20:01:26Z

client/test/request.cpp

+    std::stringstream is;
+    ArchiveLinkReqHandler req;
+
+   // ASSERT_EQ(req.buildRequestFromInput(is), REQ_SUCCESS);


пользы от таких тестов нет, к сожалению :(

ghost · 2020-12-24T14:46:59Z

Проверять с этого коммита.

leshiy1295

по ощущениям, добавилось много копипасты и "странных" решений
глобальных архитектурных проблем не решено
boost внедрён только на уровне main.cpp, что не считается эффективным и правильным

leshiy1295 · 2020-12-26T20:23:06Z

client/include/inputUtils.hpp

+std::string logInInput();
+std::string signUpInput();
+
+#include "inputUtils.tpp"


обычно это как раз-таки hpp...

leshiy1295 · 2020-12-26T20:23:18Z

client/include/inputUtils.tpp

@@ -0,0 +1,14 @@
+#pragma once
+
+#include <iostream>


зачем?

leshiy1295 · 2020-12-26T20:23:46Z

client/include/inputUtils.tpp

+template<typename T, typename... Args>
+void writeData(T* key) {
+    fillObject(key);
+}
+
+template<typename T, typename... Args>
+void writeData(T* key, Args... args) {
+    fillObject(key);
+    writeData(args...);


зачем выносить в отдельный файл?

leshiy1295 · 2020-12-26T20:23:53Z

client/include/inputUtils.tpp

+
+template<typename T, typename... Args>
+void writeData(T* key, Args... args) {
+    fillObject(key);


writeData(key);

leshiy1295 · 2020-12-26T20:24:48Z

client/include/inputUtils.tpp

+#include <iostream>
+
+template<typename T, typename... Args>
+void writeData(T* key) {


зачем?
можно было с помощью sizeof... проверить размер пакета в общем шаблоне

leshiy1295 · 2020-12-26T20:37:52Z

client/src/main.cpp

+    template<typename T>
+    bool get_value(const char* key, T& value) {
+        if (boost::optional<T> v = pt.get_optional<T>(key)) {
+            value = *v;
+            return true;
+            } else {
+                return false;
+        }
+    }
+
+    bool get_value(const char* key, std::vector<std::string>& value) {
+        if (pt.get_child_optional(key)) {
+        BOOST_FOREACH (boost::property_tree::ptree::value_type& field, pt.get_child(key))
+        {
+            value.push_back(field.second.data());
+        }
+        return true;
+    }


такой подход не считается правильным и эффективным внедрением внешних библиотек
нужно было корректно, через PIMPL, в файлах, не вынося зависимости на сторону клиента...

leshiy1295 · 2020-12-26T20:38:23Z

client/src/socket.cpp

+void Socket::Send(const std::string& str) {
+    std::string temp = "f" + str;
+    while(temp.size() != 400) {
+        temp.push_back('\x1A');


хардкод

leshiy1295 · 2020-12-26T20:39:43Z

client/src/utils.cpp

+    for(auto it = tempStr.begin(); it != tempStr.end(); it++) {
+        if ((*it == ':' || *it == ',') /* && !special_block */) {
+            elements.push_back(element);
+            element = "";


copy_if и inserter...

leshiy1295 · 2020-12-26T20:40:10Z

client/src/utils.cpp

+    for (auto it = std::find(vec.begin(), vec.end(), key) + 1; it !=  vec.end(); it++) {
+        inputVec->push_back(*it);
+    }


leshiy1295 · 2020-12-26T20:40:37Z

client/src/view.cpp

-        std::cout << "Write host of room" << std::endl;
-        std::cin >> appendStr;
-        inputStr += "*Host*" + appendStr;
+    case DELETE_ROOM: {


switch-case - это плохо в таких масштабах

kaverhovsky added 9 commits November 12, 2020 06:25

added classes and methods prototypes

9ef9705

added tests and reworked code

87ddd0e

fixed blank.yml

37715c5

fixed blank.yml 2

7fc3ae4

fixed blank.yml 3

db999d7

fixed blank.yml 4

82bde25

fixed blank.yml 5

6531e16

fixed blank.yml 6

ebfa8db

fixed blank.yml 7

24227bb

ghost requested a review from leshiy1295 November 12, 2020 14:15

ghost assigned leshiy1295 Nov 12, 2020

kaverhovsky added 4 commits November 12, 2020 17:36

removed old Cmakelist

076bc9e

fixed blank.yml 8

b8fd891

some changes

225299d

some changes2

6de03f0

leshiy1295 reviewed Nov 29, 2020

View reviewed changes

kaverhovsky added 3 commits December 10, 2020 17:22

tried to implement code

985dd79

changed blank.yml

3339e88

deleted request manager

8dc195d

leshiy1295 reviewed Dec 13, 2020

View reviewed changes

kaverhovsky added 5 commits December 24, 2020 00:55

implemented json-like serializator

f00d935

added boost parser and moved send and recieve to presenter

82043c9

changed consoleView

f14559e

login handler

c099009

add link handler

255bc0d

kaverhovsky added 3 commits December 24, 2020 19:09

changed connect

837acb2

-comms

5043f2d

--

8a30a02

kaverhovsky added 3 commits December 24, 2020 19:51

----

d90ec82

------

20ed769

improved creating and removing rooms

fe5ff7a

leshiy1295 reviewed Dec 26, 2020

View reviewed changes

kaverhovsky added 9 commits December 27, 2020 04:28

improved adding users and links

db3d341

recieving files

c9caa36

rec

d03341a

fixed serialization

a4f47cf

debug try

2fb9365

getting room from server

7f9d74c

new commit

64cf698

new

f80127f

new2

ea87c45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client #3

Client #3

ghost commented Nov 12, 2020

leshiy1295 left a comment

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

leshiy1295 Nov 29, 2020

ghost commented Dec 10, 2020

leshiy1295 left a comment

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

leshiy1295 Dec 13, 2020

ghost commented Dec 24, 2020

leshiy1295 left a comment

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

leshiy1295 Dec 26, 2020

		std::vector<std::string> tags;
		std::vector<std::string> snapshotPaths;

		@@ -0,0 +1,6 @@
		#include "userinfo.hpp"

		UserInfo::UserInfo() : name(""), password("") {}

		std::cout << "Write name of link" << std::endl;
		std::cin >> appendStr;

Client #3

Are you sure you want to change the base?

Client #3

Conversation

ghost commented Nov 12, 2020

leshiy1295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 10, 2020

leshiy1295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 24, 2020

leshiy1295 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment