-
Notifications
You must be signed in to change notification settings - Fork 0
Hw2 #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?
Hw2 #2
Changes from all commits
91ebea4
5dd8cf8
0aec724
9f7bdea
0d6fe1b
a54d6a0
80fd822
24c5feb
f622997
b8ce537
2ad924d
b2b615c
fc2ddb1
74104b3
77429af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| cmake_minimum_required(VERSION 3.15) | ||
| project(CommandParser) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 20) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| file(GLOB SOURCES "src/*.cpp") | ||
|
|
||
| # add_custom_target(run COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${EXECUTABLE} 'echo million | cat ../static/text.txt | cat ../static/zarplata.txt | wc -c | echo rambler | cat ../static/zarplata.txt | wc -c | cat ../static/text.txt | wc -c') | ||
| # MakeFile удобнее, так как там я мог make run сразу запускать файл с переданной строкой | ||
|
|
||
| add_executable(${PROJECT_NAME} ${SOURCES}) | ||
|
|
||
| target_include_directories(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include) | ||
|
|
||
| target_link_libraries(${PROJECT_NAME}) | ||
|
|
||
|
|
||
| # Автоматический запуск тестов после сборки | ||
| add_custom_command( | ||
| TARGET ${PROJECT_NAME} | ||
| POST_BUILD | ||
| COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| CXX := g++ | ||
| CXX_FLAGS := -Wall -Wextra -std=c++20 -ggdb | ||
|
|
||
| BIN := bin | ||
| SRC := src | ||
| INCLUDE := include | ||
| LIB := lib | ||
|
|
||
| LIBRARIES := | ||
| EXECUTABLE := main | ||
|
|
||
| all: $(BIN)/$(EXECUTABLE) | ||
|
|
||
| run: clean all | ||
| ./$(BIN)/$(EXECUTABLE) 'echo million | cat static/text.txt | cat static/zarplata.txt | wc -c | echo rambler | cat static/zarplata.txt | wc -c | cat static/text.txt | wc -c' | ||
|
|
||
| $(BIN)/$(EXECUTABLE): $(wildcard $(SRC)/*.cpp) | ||
| $(CXX) $(CXX_FLAGS) -I$(INCLUDE) -L$(LIB) $^ -o $@ $(LIBRARIES) | ||
|
|
||
| clean: | ||
| -rm -f $(BIN)/* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #pragma once | ||
| #include <iostream> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Вот здесь и во многих других местах, по-хорошему нужно убрать или перенести хедеры в файлы, где они действительно нужны. В этом файле, кстати, нужен только IOperation.hpp, все остальное или уже определено в IOperation.hpp или нужно, но не здесь, а например, в CatOperation.cpp |
||
| #include <memory> | ||
| #include <fstream> | ||
| #include "IOperation.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) {} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. По-моему в коде многовато "лишних" конструкторов. Например, кажется, что не нужен обычный конструктор у CatOperation(). Не вижу, где используется. Так же "хитрый" второй конструктор, с дефолтное некст-операцией, вроде тоже не нужен такой. Если где-то что-то не используется, то следует подчистить.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Реализация таких конструкторов подразумевает разное использование классов. Конструктор где второй параметр нулевой используется везде, это самый главный конструктор
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Я имею в виду: а есть ли хотя бы одно место, где этот конструктор используется с двумя параметрами? Если нет, то нужно оставить только один параметр filename и дело с концом. Кстати, почему он не const std::string& ?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. А если мне потом понадобится передать сразу при конструкторе следующую операцию? Я буду вынужден залезать в код и редактировать. Зачем? Сразу надо предусматривать удобство
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Обычно при таком подходе происходит немного не так:
Ну т.е. по многим причинам люди обычно не пишут "на будущее", "для удобства", если не знают четкого плана, когда это действительно будет нужно. Неиспользуемый код делает программу сложнее, больше, менее читаемой и сложнее поддерживаемой. |
||
|
|
||
| void ProcessLine(const std::string& pipeline) override; | ||
| void HandleEndOfInput() override; | ||
|
|
||
| private: | ||
| std::string filename_; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #pragma once | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <string> | ||
| #include "IOperation.hpp" | ||
| #include "EchoOperation.hpp" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CommandParser.hpp-заголовку не нужно знать, какие именно конкретные операции есть в программе. А заодно эти операции не нужны main.cpp, который инклудит этот CommandParser.hpp. Реально, эти заголовки нужны только CommandParser.cpp. Значит, следует туда и перенести. Это уменьшит зависимость внутри проекта, т.е. например, при добавлении нового оператора, main.cpp не нужно будет пересобирать. Т.е. скорость сборки увеличивает, размер программы итоговой уменьшает. В идеале сущности должны видеть только то, что действительно используют. Это касается и других заголовков: разного рода memory и iostream. Если конкретно здесь оно не используется - значит можно удалять. iostream, в этом файле тоже не нужен. |
||
| #include "CatOperation.hpp" | ||
| #include "WcOperation.hpp" | ||
|
|
||
| class OperationsList { | ||
| public: | ||
| OperationsList() : pipeline_(), head_(), tail_() {} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Если такой конструктор не используется в коде, то и не нужно его явно определять. То есть я бы удалил. |
||
| // input копируем, так как в далньейшем в функции он будет меняться | ||
| OperationsList(const std::string& input); | ||
|
|
||
| void AddOperation(std::shared_ptr<IOperation>&& operation); | ||
| void AddOperation(const std::string& token); | ||
| void RunOperations(); | ||
|
|
||
| private: | ||
| const std::string pipeline_; | ||
| const std::string delimiter = " | "; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Нет смысла хранить эту константу внутри класса. Ведь когда мы создаем экземпляр этого класса, эта константа будет частью получившегося объекта. И если мы создадим 2 экземпляра, то константа будет продублирована. Лучше всего этот delimeter отправить в cpp файл, где он используется, и там его можно или его вытащить в глобальные константы, или проинициализировать внутри какой-то функции, но сделаеть ее static. Оба этих варианта исключат дублирование константы в коде.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Просто сделаю ее статичной внутри класса. Тогда дублирования не будет, верно?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Верно, не будет. Но если выбирать, где лучше сделать ее статической: статическим полем класса или статической локальной переменной в одной каком-то методе в cpp-файле, то я бы выбрал второе. В этом случае мы как бы больше "скрываем" деталей реализации от других. То есть следуем правилу, по которому сущности должны видеть только то, что используют, и то, что им необходимо видеть. На практике это могло бы пригодиться в такой ситуации: решили изменить значение этого разделителя, заменили | на какой-нибудь /. Если константа определена в cpp-файле, то main.cpp перекомпилировать не пришлось бы, тк с его точки зрения, ничего не изменилось, тк он и не видел ничего. |
||
| std::shared_ptr<IOperation> head_; | ||
| std::shared_ptr<IOperation> tail_; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| #pragma once | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include "IOperation.hpp" | ||
|
|
||
| class EchoOperation final : public IOperation { | ||
| public: | ||
| EchoOperation() : IOperation() {} | ||
| EchoOperation(std::string& str, std::shared_ptr<IOperation>&& operation=nullptr) : IOperation(std::move(operation)), str_(str) {} | ||
|
|
||
| void ProcessLine(const std::string& pipeline) override; | ||
| void HandleEndOfInput() override; | ||
|
|
||
| private: | ||
| std::string str_; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #pragma once | ||
| #include <iostream> | ||
| #include <memory> | ||
|
|
||
| class IOperation { | ||
| public: | ||
| IOperation() : next_operation_() {} | ||
| IOperation(std::shared_ptr<IOperation>&& operation) : next_operation_(std::move(operation)) {} | ||
|
|
||
| virtual void ProcessLine(const std::string& pipeline) = 0; | ||
| virtual void HandleEndOfInput() = 0; | ||
| virtual void SetNextOperation(std::shared_ptr<IOperation>&& operation); | ||
|
|
||
| virtual ~IOperation() {} | ||
|
|
||
| std::shared_ptr<IOperation> next_operation_; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #pragma once | ||
| #include <iostream> | ||
| #include <memory> | ||
| #include <fstream> | ||
| #include <sstream> | ||
| #include "IOperation.hpp" | ||
|
|
||
| class WcOperation final : public IOperation { | ||
| public: | ||
| WcOperation() : IOperation() {} | ||
| WcOperation(std::string& str, std::shared_ptr<IOperation>&& operation=nullptr) : IOperation(std::move(operation)), str_(str) {} | ||
|
|
||
| void ProcessLine(const std::string& pipeline) override; | ||
| void HandleEndOfInput() override; | ||
|
|
||
| private: | ||
| std::string str_; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #include "CatOperation.hpp" | ||
| #include <sstream> | ||
|
|
||
| void CatOperation::ProcessLine(const std::string& pipeline) { | ||
| std::ifstream file_(filename_); | ||
| if (!file_.is_open()) { | ||
| throw std::runtime_error("Cannot open file: " + filename_); | ||
| } | ||
|
|
||
| // создаем std::stringstream и передаем в него буфер файла | ||
| std::stringstream file_stream; | ||
| file_stream << file_.rdbuf(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 слова о подводных камнях. |
||
|
|
||
| // получаем содержимое файла в виде строки | ||
| std::string file_contents = file_stream.str(); | ||
|
|
||
| // передаем pipeline + file_contents в метод ProcessLine следующей операции | ||
| if (next_operation_) { | ||
| next_operation_->ProcessLine(pipeline + file_contents); | ||
| } else { | ||
| std::cout << pipeline << std::endl << file_contents << std::endl; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| void CatOperation::HandleEndOfInput() { | ||
| ProcessLine(""); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| #include "CommandParser.hpp" | ||
|
|
||
| OperationsList::OperationsList(const std::string& input) : pipeline_(input), head_(nullptr), tail_(nullptr) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Я бы не стал явно инициализировать поля, для которых мы и так знаем, что делает конструктор по умолчанию. Т.е. head_ и tail_ и так понятно, что будут нулями. Можно убрать из листа явной инициализации. (Другое дело, когда мы имеем дело, с какими-нибудь интами, и там не опредлено, какое значение будет в поле, если его явно не инициализировали, да потом еще и обратились к ней, не проинциализировав.) |
||
| size_t pos = 0; | ||
| std::string token; | ||
| std::string_view cursor(pipeline_); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Я вижу, что pipeline_ используется только 1 раз, в конструкторе. Если так, то зачем его хранить, тратить место, и ломать голову, зачем он нужен. Не стоит добавлять в класс методы и поля "на будущее". Если они не используются - то надо просто удалить. Так код становится более читаемым и оптимальным. |
||
|
|
||
| // Пока есть разделители, запоминаем позицию | ||
| while ((pos = cursor.find(delimiter)) != std::string::npos) { | ||
| // Получаем подстроку до разделителя | ||
| token = cursor.substr(0, pos); | ||
| // Удаляем ее из pipeline_ учитывая длину разделителя | ||
| cursor = cursor.substr(pos + delimiter.length()); | ||
| AddOperation(token); | ||
| } | ||
|
|
||
| // Последняя команда указана без разделителя, обрабатываем ее | ||
| if (!cursor.empty()) { | ||
| AddOperation(std::string(cursor)); | ||
| } | ||
| } | ||
|
|
||
| void OperationsList::AddOperation(const std::string& token){ | ||
| const std::string ECHO = "echo"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Вот эти константы лучше или отправить вне функции просто наверх, или оставить здесь, но static. Так они не будут создаваться и удаляться при каждой AddOperation. А еще лучше не использовать капслок для обычных констант. Это похоже на дефайны, но все-таки не дефайны.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. А какие константы считаются необычными?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Неверно выразился. Я имею в виду любые константы. В противовес старым дефайнам. Раньше в Сишном коде повсюду были дефайны и разные макросы. Сейчас уже язык богаче, возможностей больше, и использовать старомодные дефайны считается "моветоном". А капслок в названии - это некое подражание старому стилю. Но его использовать сегодня незачем, капслок - только для дефайнов, а вместо дефайнов теперь константы. |
||
| const std::string CAT = "cat"; | ||
| const std::string WC = "wc -c"; | ||
|
|
||
| if (token.starts_with(ECHO)) { | ||
| std::string str = token.substr(ECHO.length() + 1); // Достаем переданную строку | ||
| AddOperation(std::make_shared<EchoOperation>(str)); | ||
| return; | ||
| } | ||
|
|
||
| if (token.starts_with(CAT)) { | ||
| std::string file = token.substr(CAT.length() + 1); // Достаем переданный файл | ||
| AddOperation(std::make_shared<CatOperation>(file)); | ||
| return; | ||
| } | ||
|
|
||
| if (token.starts_with(WC)) { | ||
| std::string str; | ||
| if (token.length() > WC.length()) { | ||
| str = token.substr(WC.length() + 1); // Достаем переданную строку | ||
| } | ||
| AddOperation(std::make_shared<WcOperation>(str)); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| void OperationsList::AddOperation(std::shared_ptr<IOperation>&& operation) { | ||
| if (!head_) { | ||
| head_ = std::move(operation); | ||
| tail_ = head_; | ||
| } else { | ||
| tail_->SetNextOperation(std::move(operation)); | ||
| tail_ = tail_->next_operation_; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| void OperationsList::RunOperations() { | ||
| if (head_) { | ||
| head_->HandleEndOfInput(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| #include "EchoOperation.hpp" | ||
|
|
||
| void EchoOperation::ProcessLine(const std::string& pipeline) { | ||
| if(next_operation_) { | ||
| next_operation_->ProcessLine(str_); | ||
| } else { | ||
| std::cout << str_ << std::endl; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| void EchoOperation::HandleEndOfInput() { | ||
| ProcessLine(""); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| #include "IOperation.hpp" | ||
|
|
||
| // Реализуем по умолчанию метод интерфейсного класса | ||
| void IOperation::SetNextOperation(std::shared_ptr<IOperation>&& operation) { | ||
| next_operation_ = std::move(operation); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Хорошая идея выделить общий код в базовый класс, чтобы избежать дублирования в каждом из наследников, где этот же код нужен именно в таком виде. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #include "WcOperation.hpp" | ||
|
|
||
| void WcOperation::ProcessLine(const std::string& pipeline) { | ||
| // вычисляем суммарное число байт в строке full_string = pipeline + str_ | ||
| int total_bytes = sizeof(char) * (pipeline.size() + str_.size()); | ||
|
|
||
| // передаем pipeline + str_ в метод ProcessLine следующей операции | ||
| if (next_operation_) { | ||
| next_operation_->ProcessLine(std::to_string(total_bytes)); | ||
| } else { | ||
| std::cout << total_bytes << std::endl; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| void WcOperation::HandleEndOfInput() { | ||
| ProcessLine(""); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #include <iostream> | ||
| #include "CommandParser.hpp" | ||
|
|
||
|
|
||
| int main(int argc, char* argv[]) { | ||
| if (argc != 2) { | ||
| std::cerr << "Run program with arguments like: 'echo <someText> | cat <someFileName> | echo million | cat Elon.txt'\n"; | ||
| return 1; | ||
| } | ||
| try{ | ||
| OperationsList list(argv[1]); | ||
| list.RunOperations(); | ||
| } | ||
| catch(const std::exception& e){ | ||
| std::cerr << e.what() << '\n'; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. retrun 1; (раз какая-то ошибка) |
||
| } | ||
|
|
||
|
|
||
| return 0; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.