Hw2 by TimofeyTst · Pull Request #2 · TimofeyTst/VKML_AdvancedCPP

TimofeyTst · 2023-03-29T19:03:58Z

No description provided.

Задал вопрос разным людям Все молчат. Я злой

TimofeyTst · 2023-03-30T13:36:11Z

Единственное не могу понять: Логику методов класса из файла CommandParser.hpp я вынес отдельно в файл CommanParser.cpp, в котором сделал #include "CommandParser.hpp", и у меня все отлично компилируется.
Я захотел вынести логику методов класса IOperation в отдельный файл IOperation.cpp, но когда я создаю src/IOperation.cpp и делаю первой строкой #include "IOperation.hpp" то файл не видит такого заголовочного файла, хотя он есть. Вроде мой makefile учитывает все эти заголовочные файлы, но почему так происходит я не мог понять, мучился около часа с этим, даже проигнорировал сообщение от vscode о том, что нет связи, просто вручную перенес в этот файл логику, но компиляция не прошла, почему такое может быть?

0x0000dead · 2023-03-31T03:02:32Z

Скорее всего, потому что у тебя поиск hpp файла происходит в директории src, а он в "../include/*.hpp". Можешь попробовать использовать CMakeLists.txt - кроссплатформенно и понятнее

TimofeyTst · 2023-04-02T07:35:20Z

Скорее всего, потому что у тебя поиск hpp файла происходит в директории src, а он в "../include/*.hpp". Можешь попробовать использовать CMakeLists.txt - кроссплатформенно и понятнее

Не знаю что я сделал, но оно заработало. Перенес логику hpp в cpp

0x0000dead

merge конфликт какой-то, а так по логике задания вроде нормально

include/IOperation.hpp

src/IOperation.cpp

src/CommandParser.cpp

src/main.cpp

src/CommandParser.cpp

include/CommandParser.hpp

src/CommandParser.cpp

include/CatOperation.hpp

src/CommandParser.cpp

Makefile

README.md

0x0000dead

LGTM, только конфликт с README

Sending prev pipeline to next operation

ikramichev · 2023-04-21T12:07:09Z

src/main.cpp

+        list.RunOperations();
+    }
+    catch(const std::exception& e){
+        std::cerr << e.what() << '\n';


retrun 1; (раз какая-то ошибка)

ikramichev · 2023-04-21T12:15:31Z

include/CommandParser.hpp

+#include <memory>
+#include <string>
+#include "IOperation.hpp"
+#include "EchoOperation.hpp"


CommandParser.hpp-заголовку не нужно знать, какие именно конкретные операции есть в программе. А заодно эти операции не нужны main.cpp, который инклудит этот CommandParser.hpp. Реально, эти заголовки нужны только CommandParser.cpp. Значит, следует туда и перенести. Это уменьшит зависимость внутри проекта, т.е. например, при добавлении нового оператора, main.cpp не нужно будет пересобирать. Т.е. скорость сборки увеличивает, размер программы итоговой уменьшает. В идеале сущности должны видеть только то, что действительно используют. Это касается и других заголовков: разного рода memory и iostream. Если конкретно здесь оно не используется - значит можно удалять. iostream, в этом файле тоже не нужен.

ikramichev · 2023-04-21T12:18:47Z

include/CommandParser.hpp

+
+private:
+    const std::string pipeline_;
+    const std::string delimiter = " | ";


Нет смысла хранить эту константу внутри класса. Ведь когда мы создаем экземпляр этого класса, эта константа будет частью получившегося объекта. И если мы создадим 2 экземпляра, то константа будет продублирована. Лучше всего этот delimeter отправить в cpp файл, где он используется, и там его можно или его вытащить в глобальные константы, или проинициализировать внутри какой-то функции, но сделаеть ее static. Оба этих варианта исключат дублирование константы в коде.

Просто сделаю ее статичной внутри класса. Тогда дублирования не будет, верно?

Верно, не будет. Но если выбирать, где лучше сделать ее статической: статическим полем класса или статической локальной переменной в одной каком-то методе в cpp-файле, то я бы выбрал второе. В этом случае мы как бы больше "скрываем" деталей реализации от других. То есть следуем правилу, по которому сущности должны видеть только то, что используют, и то, что им необходимо видеть. На практике это могло бы пригодиться в такой ситуации: решили изменить значение этого разделителя, заменили | на какой-нибудь /. Если константа определена в cpp-файле, то main.cpp перекомпилировать не пришлось бы, тк с его точки зрения, ничего не изменилось, тк он и не видел ничего.

ikramichev · 2023-04-21T12:21:29Z

include/CommandParser.hpp

+
+class OperationsList {
+public:
+    OperationsList() : pipeline_(), head_(), tail_() {}


Если такой конструктор не используется в коде, то и не нужно его явно определять. То есть я бы удалил.
Даже если бы такой вариант конструктора был бы востребован, то я бы удалил лист инициализации после двоеточия. Ведь каждый из этих параметров имеет известный нам конструктор по умолчанию, который не нужно явно вызывать, и значения по умолчанию всех 3х полей нам известны.

ikramichev · 2023-04-21T12:24:24Z

src/CommandParser.cpp

+OperationsList::OperationsList(const std::string& input) : pipeline_(input), head_(nullptr), tail_(nullptr) {
+    size_t pos = 0;
+    std::string token;
+    std::string_view cursor(pipeline_);


Я вижу, что pipeline_ используется только 1 раз, в конструкторе. Если так, то зачем его хранить, тратить место, и ломать голову, зачем он нужен. Не стоит добавлять в класс методы и поля "на будущее". Если они не используются - то надо просто удалить. Так код становится более читаемым и оптимальным.

ikramichev · 2023-04-21T12:26:18Z

src/CommandParser.cpp

@@ -0,0 +1,66 @@
+#include "CommandParser.hpp"
+
+OperationsList::OperationsList(const std::string& input) : pipeline_(input), head_(nullptr), tail_(nullptr) {


Я бы не стал явно инициализировать поля, для которых мы и так знаем, что делает конструктор по умолчанию. Т.е. head_ и tail_ и так понятно, что будут нулями. Можно убрать из листа явной инициализации. (Другое дело, когда мы имеем дело, с какими-нибудь интами, и там не опредлено, какое значение будет в поле, если его явно не инициализировали, да потом еще и обратились к ней, не проинциализировав.)

ikramichev · 2023-04-21T13:02:33Z

src/CommandParser.cpp

+}
+
+void OperationsList::AddOperation(const std::string& token){
+    const std::string ECHO = "echo";


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

А еще лучше не использовать капслок для обычных констант. Это похоже на дефайны, но все-таки не дефайны.

А какие константы считаются необычными?

Неверно выразился. Я имею в виду любые константы. В противовес старым дефайнам. Раньше в Сишном коде повсюду были дефайны и разные макросы. Сейчас уже язык богаче, возможностей больше, и использовать старомодные дефайны считается "моветоном". А капслок в названии - это некое подражание старому стилю. Но его использовать сегодня незачем, капслок - только для дефайнов, а вместо дефайнов теперь константы.

ikramichev · 2023-04-21T13:09:17Z

src/IOperation.cpp

+
+// Реализуем по умолчанию метод интерфейсного класса
+void IOperation::SetNextOperation(std::shared_ptr<IOperation>&& operation) {
+    next_operation_ = std::move(operation);


Хорошая идея выделить общий код в базовый класс, чтобы избежать дублирования в каждом из наследников, где этот же код нужен именно в таком виде.
Но в данном случае, чище было бы оставить интерфейсный класс вообще без определений, т.е. эту функцию оставить = 0, а вот между IOperation и конкретными операциями сделать промежуточный класс, в котором был бы собран весь общий код для всех операций. В результате решение было бы гибче.

ikramichev · 2023-04-21T13:12:00Z

include/CatOperation.hpp

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


Вот здесь и во многих других местах, по-хорошему нужно убрать или перенести хедеры в файлы, где они действительно нужны. В этом файле, кстати, нужен только IOperation.hpp, все остальное или уже определено в IOperation.hpp или нужно, но не здесь, а например, в CatOperation.cpp

ikramichev · 2023-04-21T13:14:55Z

include/CatOperation.hpp

+class CatOperation final : public IOperation {
+public:
+    CatOperation() : IOperation() {}
+    CatOperation(std::string& filename, std::shared_ptr<IOperation>&& operation=nullptr) : IOperation(std::move(operation)), filename_(filename) {}


По-моему в коде многовато "лишних" конструкторов. Например, кажется, что не нужен обычный конструктор у CatOperation(). Не вижу, где используется. Так же "хитрый" второй конструктор, с дефолтное некст-операцией, вроде тоже не нужен такой. Если где-то что-то не используется, то следует подчистить.

Реализация таких конструкторов подразумевает разное использование классов. Конструктор где второй параметр нулевой используется везде, это самый главный конструктор

Я имею в виду: а есть ли хотя бы одно место, где этот конструктор используется с двумя параметрами? Если нет, то нужно оставить только один параметр filename и дело с концом. Кстати, почему он не const std::string& ?
Я имею в виду, что если что-то не используется, то оно должно быть удалено.

А если мне потом понадобится передать сразу при конструкторе следующую операцию? Я буду вынужден залезать в код и редактировать. Зачем? Сразу надо предусматривать удобство

Обычно при таком подходе происходит немного не так:

Код так и остается неиспользованным.

Когда действительно понадобится что-то поменять, то скорее всего не именно здесь, а немного рядом (например, действительно потребуется второй параметр, но другой), и в этом случае придется не только добавлять второй нужный параметр, но и продолжать поддерживать ненужный третий.

Читается такой код хуже. Будет потрачено время на анализ, что это и для чего. (не обязательно к этому случаю, а просто к любому неиспользумому коду). Если потребуется код "переписать", то много усилий стоит понять, для чего же нужен код, который не используется, и можно ли его просто удалить.

Работает ли правильно неиспользуемый код - большой вопрос. Можно ли его просто взять и использовать. Я имею в виду случай, когда программа давно живет "в продакшне", и в ней нужно что-то починить или добавить. Используемый код так или иначе проверен реальной работой.

Если оставить, допустим, такую возможность в конструкторе. То почему не добавить еще несколько конструкторов? Если мы пишем библиотеку, вроде STL, то, конечно, нам надо предоставить все виды конструкторов, которые могли бы быть полезны пользователю. Но в нашем случае мы пишем прикладную программу.

Ну т.е. по многим причинам люди обычно не пишут "на будущее", "для удобства", если не знают четкого плана, когда это действительно будет нужно. Неиспользуемый код делает программу сложнее, больше, менее читаемой и сложнее поддерживаемой.

ikramichev · 2023-04-21T13:18:45Z

src/CatOperation.cpp

+
+    // создаем std::stringstream и передаем в него буфер файла
+    std::stringstream file_stream;
+    file_stream << file_.rdbuf();


2 слова о подводных камнях.
Вижу, что этот вариант cat-а читает файл целиком, а потом целиком отправляет его дальше. Настоящий линуксовый кат, читает файл все-таки построчно, и построчно отправляет его на вывод. Благодаря этой особенности, можно дампить гигантские файлы, или вообще обрабатывать гигантские файлы, например грепом, построчно. Если прочитать слишком большой файл, то можно "надорваться".

TimofeyTst added 7 commits March 26, 2023 22:43

Несколько раз перечитал задание

91ebea4

Задал вопрос разным людям Все молчат. Я злой

Update readme

5dd8cf8

Update readme

0aec724

Update readme

9f7bdea

Sigmentation fault

0d6fe1b

Stable v1

a54d6a0

Some comments

80fd822

TimofeyTst requested a review from 0x0000dead March 29, 2023 19:03

move logic hpp to IOperation.cpp

24c5feb

0x0000dead reviewed Apr 2, 2023

View reviewed changes

TimofeyTst added 3 commits April 3, 2023 15:14

Each class in own hpp & shared ptrs

f622997

Some factroing

b8ce537

adding string_view

2ad924d

TimofeyTst requested a review from 0x0000dead April 3, 2023 12:50

0x0000dead reviewed Apr 5, 2023

View reviewed changes

include/CatOperation.hpp Outdated Show resolved Hide resolved

src/CommandParser.cpp Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

TimofeyTst added 3 commits April 5, 2023 21:47

CMake, final, C++20

b2b615c

wc -c Operation

fc2ddb1

some clean code

74104b3

TimofeyTst requested a review from 0x0000dead April 5, 2023 19:33

0x0000dead approved these changes Apr 11, 2023

View reviewed changes

TimofeyTst requested a review from ikramichev April 12, 2023 10:36

Version 3.0

77429af

Sending prev pipeline to next operation

ikramichev reviewed Apr 21, 2023

View reviewed changes

ikramichev approved these changes Apr 21, 2023

View reviewed changes

		@@ -0,0 +1,66 @@
		#include "CommandParser.hpp"

		OperationsList::OperationsList(const std::string& input) : pipeline_(input), head_(nullptr), tail_(nullptr) {

Comments

Conversation

TimofeyTst commented Mar 29, 2023

Uh oh!

TimofeyTst commented Mar 30, 2023

Uh oh!

0x0000dead commented Mar 31, 2023

Uh oh!

TimofeyTst commented Apr 2, 2023

Uh oh!

0x0000dead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0x0000dead left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ikramichev Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

3 participants

0x0000dead left a comment •

edited

Loading

ikramichev Apr 21, 2023 •

edited

Loading