Кайгородов Александр #28
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.
Создавать переменные с непонятными именами излишне, в данном случае эффективнее и читабельнее написать выражение непосредственно в строке return. Причем будет достаточно одного каста, поскольку второй аргумент будет приведен неявно.
| } | ||
| std::cout << "DEST"; | ||
| prev_print = 1; | ||
| } |
There was a problem hiding this comment.
Много дублирования, можно было вынести общие части придумав другую логику
| 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.
не хватает пробала перед {
| void SwapPtr(/* write arguments here */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| void SwapPtr(int*& ptr1, int*& ptr2) { |
There was a problem hiding this comment.
обыычно принято называть lhs, rhs (left/right hand side) что говорит левый и правый аргумент и короче
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| void SwapPtr(int*& ptr1, int*& ptr2) { | ||
| int* ptr0 = ptr1; |
There was a problem hiding this comment.
вместо ptr0, комфортнее было бы temp, что явно говорит, что это временная переменная и нет необходимости искать глазами цифру среди ptr
| DataStats CalculateDataStats(std::vector<int>& vect) { | ||
| int n = 0; | ||
|
|
||
| DataStats rezult = {0, 0}; |
There was a problem hiding this comment.
result - не нужно русифицировать английский
|
|
||
| /* return_type */ CalculateDataStats(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| DataStats CalculateDataStats(std::vector<int>& vect) { |
There was a problem hiding this comment.
обычно называют values
| rezult.avg /= n; | ||
| for (auto it = vect.begin(); it != vect.end(); ++it) { | ||
| rezult.sd += pow((*it - rezult.avg), 2); | ||
| } |
There was a problem hiding this comment.
Два цикла, есть возможность считать это в одном цикле, при большом количестве элементов два прохода может быть заметно накладнее одного
| rezult.sd += pow((*it - rezult.avg), 2); | ||
| } | ||
| rezult.sd /= n; | ||
| rezult.sd = sqrt(rezult.sd); |
| return rezult; | ||
| } | ||
|
|
||
| DataStats CalculateDataStats(const std::vector<int>& vect) { |
There was a problem hiding this comment.
- полное дублирование кода,
- поскольку мы не меняем исходные данные, должна быть только константная версия аргумента
|
|
||
| bool operator<(Date date1, Date date2) { | ||
| return std::tie(date1.year, date1.month, date1.day) | ||
| < std::tie(date2.year, date2.month, date2.day); |
There was a problem hiding this comment.
делают отступ в 4 пробела, когда инструкция продолжается на следующей строке
| } | ||
|
|
||
| bool operator>(Date date1, Date date2) { | ||
| return !(date1 < date2) and !(date1 == date2); |
There was a problem hiding this comment.
принято использовать операторы && и ||
| 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.
зачем повторять если можно было выразить через предыдущий
| if (st1.birth_date != st2.birth_date) { | ||
| return st1.birth_date < st2.birth_date; | ||
| } | ||
| else |
| } | ||
| else | ||
| return false; | ||
|
|
| 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.
Так не плохо, но в данном случае также можно было бы воспользоваться std::tie в левом операнде аргументом могут быть как st1 так и st2 что позволит выразить как < так и > используя в конечном счете выражение std::tie(st1..., st2....) < std::tie(st1..., st2....)
| /* return_type */ Filter(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| std::vector<int> Filter(std::vector<int>& vec, bool (*func) (int)) { |
There was a problem hiding this comment.
зачем возвращать копию? в данном случае функция меняет входной аргумент vec, следовательно возвращать должна void, иначе излишне копируем
| } | ||
| vec_out.shrink_to_fit(); | ||
| return vec_out; | ||
| } |
There was a problem hiding this comment.
Достаточно одной функции которая принимает константный вектор и возвращает неконстантный вектор позиций,
| /* return_type */ FindAll(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| std::vector<size_t> FindAll(std::vector<int>& vec, bool (*func) (int)) { |
There was a problem hiding this comment.
изменения вектора функцией не производятся, некорректно передавать вектор без константности, так как это запутывает смысл
| return vec_out; | ||
| } | ||
|
|
||
| const std::vector<size_t> FindAll(const std::vector<int>& vec, bool (*func) (int)) { |
There was a problem hiding this comment.
зачем возвращать константный вектор? пользователь может захотеть его изменить, в данном случае это излишняя константность на возвращаемом значении
|
|
||
| /* 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.
это не корректная сигнатура. Данной функции не должно быть, поскольку поиск максимума не изменяет элементы контейнера, вектор должен передаваться константной ссылкой. Итераторы лучше сделать константными, так как не было дополнительного требования, что по ним можно менять элементы
| /* 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.
принято писать 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.
можно проинициализировать пару сразу
| } | ||
|
|
||
| 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.
лучше проинициализировать пару сразу
и если использовать элементы пары сразу как min_it, max_it можно убрать несколько строк кода
либо создавать пару непосредственно при возврате из функции, чтобы не создавать лишние переменные, сейчас под одно назначение выделена двойная память под res и под min_it и max_it
| } | ||
| else{ | ||
| os << "-"; | ||
| } |
There was a problem hiding this comment.
в данном случае идеально подходит тернарный оператор
| } | ||
| else{ | ||
| os << "circle[]"; | ||
| } |
There was a problem hiding this comment.
в данном случае идеально подходит тернарный оператор
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegionList& list) { | ||
| size_t size_list = size(list); |
There was a problem hiding this comment.
у вектора есть метод .size() корректней использовать его нежели вызывать функцию std::size
| return os; | ||
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegionList& list) { |
There was a problem hiding this comment.
некорректная сигнатура, CircleRegionList - должен быть константным так как неизменяется
| return os; | ||
| } | ||
|
|
||
| std::ostream& operator<<(std::ostream& os, CircleRegion& region) { |
There was a problem hiding this comment.
CircleRegion - должен быть константным
| } | ||
| os << "}"; | ||
| return os; | ||
| } |
There was a problem hiding this comment.
некорректное дублирование функций путем создания неконстантных версий
| return vec_out; | ||
| } | ||
|
|
||
| const std::vector<int> Unique(const std::vector<int>& vec_in) { |
There was a problem hiding this comment.
должна быть одна версия которая возвраает обычный вектор и принимает константную ссылку.
излишнее дублирование, некорректная сигнатура функций
| if (elem_in_in == false){ | ||
| vec_out.push_back(elem); | ||
| } | ||
| } |
There was a problem hiding this comment.
алгоритм сомнительный
18thday
left a comment
18thday
left a comment
There was a problem hiding this comment.
@kaigorodovMM основные моменты:
- использование постфиксного инкремента вместо префиксного
- UB const_cast
- использование const_cast там, где не нужно
- именование переменных (русифицированное транскриптом или не отражающее суть)
- много некорректных сигнатур функций (недостающая или излишняя константность, дублирование функций)
2 participants
No description provided.