-
Notifications
You must be signed in to change notification settings - Fork 33
Кайгородов Александр #28
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
github.com:psds-cpp/psds-cpp-2025
| throw std::runtime_error{"Not implemented"}; | ||
| int64_t c = static_cast<int64_t>(a); | ||
| int64_t d = static_cast<int64_t>(b); | ||
| return c + d; |
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.
Создавать переменные с непонятными именами излишне, в данном случае эффективнее и читабельнее написать выражение непосредственно в строке return. Причем будет достаточно одного каста, поскольку второй аргумент будет приведен неявно.
| } | ||
| std::cout << "DEST"; | ||
| prev_print = 1; | ||
| } |
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 (flags <= CheckFlags::ALL && flags >= CheckFlags::NONE){ | ||
| std::cout << "["; | ||
| bool prev_print = 0; | ||
| if (static_cast<uint8_t>(flags) & static_cast<uint8_t>(CheckFlags::TIME)){ |
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.
не хватает пробала перед {
| } | ||
|
|
||
| bool operator>(Date date1, Date date2) { | ||
| return !(date1 < date2) and !(date1 == date2); |
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.
принято использовать операторы && и ||
| return std::tie(st1.mark, st1.score) == std::tie(st2.mark, st2.score); | ||
| } | ||
| bool operator!=(StudentInfo st1, StudentInfo st2) { | ||
| return !(std::tie(st1.mark, st1.score) == std::tie(st2.mark, st2.score)); |
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 (st1.birth_date != st2.birth_date) { | ||
| return st1.birth_date < st2.birth_date; | ||
| } | ||
| 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.
else тут лишний
| } | ||
| else | ||
| return false; | ||
|
|
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.
лишняя строка
| return !(std::tie(st1.mark, st1.score) == std::tie(st2.mark, st2.score)); | ||
| } | ||
|
|
||
| bool operator<(StudentInfo st1, StudentInfo st2) { |
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.
Так не плохо, но в данном случае также можно было бы воспользоваться std::tie в левом операнде аргументом могут быть как st1 так и st2 что позволит выразить как < так и > используя в конечном счете выражение std::tie(st1..., st2....) < std::tie(st1..., st2....)
| /* return_type */ Filter(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| std::vector<int> Filter(std::vector<int>& vec, bool (*func) (int)) { |
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.
зачем возвращать копию? в данном случае функция меняет входной аргумент vec, следовательно возвращать должна void, иначе излишне копируем
| } | ||
| vec_out.shrink_to_fit(); | ||
| return vec_out; | ||
| } |
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.
Достаточно одной функции которая принимает константный вектор и возвращает неконстантный вектор позиций,
| /* return_type */ FindAll(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| std::vector<size_t> FindAll(std::vector<int>& vec, bool (*func) (int)) { |
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.
изменения вектора функцией не производятся, некорректно передавать вектор без константности, так как это запутывает смысл
| return vec_out; | ||
| } | ||
|
|
||
| const std::vector<size_t> FindAll(const std::vector<int>& vec, bool (*func) (int)) { |
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.
зачем возвращать константный вектор? пользователь может захотеть его изменить, в данном случае это излишняя константность на возвращаемом значении
|
|
||
| /* return_type */ MinMax(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| std::pair <std::vector<int>::iterator, std::vector<int>::iterator> MinMax(std::vector<int>& vec) { |
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.
это не корректная сигнатура. Данной функции не должно быть, поскольку поиск максимума не изменяет элементы контейнера, вектор должен передаваться константной ссылкой. Итераторы лучше сделать константными, так как не было дополнительного требования, что по ним можно менять элементы
| /* return_type */ MinMax(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| std::pair <std::vector<int>::iterator, std::vector<int>::iterator> MinMax(std::vector<int>& vec) { | ||
| std::pair<std::vector<int>::iterator, std::vector<int>::iterator> rez; |
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.
принято писать result на английском, либо res
| std::pair<std::vector<int>::iterator, std::vector<int>::iterator> rez; | ||
| if (vec.begin() == vec.end()){ | ||
| rez.first = vec.begin(); | ||
| rez.second = vec.begin(); |
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.
можно проинициализировать пару сразу
| } | ||
|
|
||
| std::pair <std::vector<int>::const_iterator, std::vector<int>::const_iterator> MinMax(const std::vector<int>& vec) { | ||
| std::pair<std::vector<int>::const_iterator, std::vector<int>::const_iterator> rez; |
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.
лучше проинициализировать пару сразу
и если использовать элементы пары сразу как min_it, max_it можно убрать несколько строк кода
либо создавать пару непосредственно при возврате из функции, чтобы не создавать лишние переменные, сейчас под одно назначение выделена двойная память под res и под min_it и max_it
| } | ||
| else{ | ||
| os << "-"; | ||
| } |
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.
в данном случае идеально подходит тернарный оператор
| } | ||
| else{ | ||
| os << "circle[]"; | ||
| } |
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.
в данном случае идеально подходит тернарный оператор
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegionList& list) { | ||
| size_t size_list = size(list); |
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.
у вектора есть метод .size() корректней использовать его нежели вызывать функцию std::size
| return os; | ||
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegionList& list) { |
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.
некорректная сигнатура, CircleRegionList - должен быть константным так как неизменяется
| return os; | ||
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegion& region) { |
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.
CircleRegion - должен быть константным
| } | ||
| os << "}"; | ||
| return os; | ||
| } No newline at end of file |
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.
некорректное дублирование функций путем создания неконстантных версий
| return vec_out; | ||
| } | ||
|
|
||
| const std::vector<int> Unique(const std::vector<int>& vec_in) { |
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 (elem_in_in == false){ | ||
| vec_out.push_back(elem); | ||
| } | ||
| } |
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.
алгоритм сомнительный
18thday
left a comment
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.
@kaigorodovMM основные моменты:
- использование постфиксного инкремента вместо префиксного
- UB const_cast
- использование const_cast там, где не нужно
- именование переменных (русифицированное транскриптом или не отражающее суть)
- много некорректных сигнатур функций (недостающая или излишняя константность, дублирование функций)
| class Queue { | ||
| public: | ||
| Queue() = default; | ||
| Queue(std::vector<int>& vec); |
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.
константная ссылка должна быть
| public: | ||
| Queue() = default; | ||
| Queue(std::vector<int>& vec); | ||
| Queue(std::stack<int>& s); |
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.
лучше принять по значению, иначе это удивительно, что при конструировании объекта исходный опусташается
| std::vector<int> vec_in; | ||
| std::vector<int> vec_out; | ||
| }; | ||
| Queue::Queue(std::vector<int>& vec) { |
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.
нужно воспользоваться списком инициализации и конструктором вектора от вектора
| st.pop(); | ||
| } | ||
| } | ||
| Queue::Queue(std::initializer_list<int> init_list) { |
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.
у вектора есть конструктор от initializer_list и также лучше использовать список инициалзиации
| while (!vec_in.empty()) { | ||
| vec_out.push_back(vec_in.back()); | ||
| vec_in.pop_back(); | ||
| } |
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 (!vec_in.empty()){ | ||
| return vec_in.back(); | ||
| } | ||
| return vec_out.front(); |
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.
тернарный оператор в одну строку
| } | ||
|
|
||
| bool Queue::operator==(const Queue& other_queue) const { | ||
| return other_queue.vec_in == vec_in && other_queue.vec_out == vec_out; |
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.
это некорректное сравнение, поскольку при равенстве элементов они могут по разному располагаться в контейнерах внутренних, нужно это учитывать
18thday
left a comment
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.
@kaigorodovMM в задачах по 4 части:
- использовать список инициализации у конструкторов
- решено мало заданий
No description provided.