Петров Николай #37
Петров Николай #37
Conversation
| } | ||
| if (static_cast<uint8_t>(flags & CheckFlags::DEST)) { | ||
| active_flags.push_back("DEST"); | ||
| } |
There was a problem hiding this comment.
по сути это дублирование, которое можно было избежать
| if (predicate(container[i])) { | ||
| ++count; | ||
| } | ||
| } |
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> |
There was a problem hiding this comment.
должны быть скорее константные итераторы, поскольку требований к их изменению не было.
также лучше обзавестись псевдонимом для итераторов, чтобы тип не выглядел громоздко
|
|
||
| // Константная версия (для работы с const vector) | ||
| std::pair<std::vector<int>::const_iterator, std::vector<int>::const_iterator> | ||
| MinMax(const std::vector<int>& vec) { |
There was a problem hiding this comment.
полное дублирование кода получается
| std::ostream& operator<<(std::ostream& os, const CircleRegionList& list) { | ||
| if (list.empty()) { | ||
| os << "{}"; | ||
| } else { |
There was a problem hiding this comment.
нужно из условия выше сразу выйти и убрать лишнюю вложенность
|
|
||
| if ((step > 0 && from >= to) || (step < 0 && from <= to)) { | ||
| return std::vector<int>(); | ||
| } |
There was a problem hiding this comment.
можно объединить условие с условием выше
| count = (to - from - 1) / step + 1; | ||
| } else { | ||
| count = (to - from + 1) / step + 1; | ||
| } |
There was a problem hiding this comment.
это тернарный оператор при инициализации переменной, причем можно +1 -1 только его использовать
| while (current > to) { | ||
| result.push_back(current); | ||
| current += step; | ||
| } |
There was a problem hiding this comment.
это тоже дублирование
| return result; | ||
| } | ||
|
|
||
| std::vector<int> Range(int from, int to) { |
There was a problem hiding this comment.
это лучше делать аргументом по умолчанию у исходной функции, а не пистаь явно функцию
| } | ||
| std::vector<int> Unique(const std::vector<int>& sorted_vec) { | ||
| if (sorted_vec.empty()) { | ||
| return std::vector<int>(); |
There was a problem hiding this comment.
return {}; тип возвращаемого значения написан, поэтому не нужно его писать заново
|
|
||
| // Бинарные арифметические операторы | ||
|
|
||
| Phasor operator + (const Phasor& a, const Phasor& b) { |
There was a problem hiding this comment.
обычно написание операторов соответствует написанию функций, без пробелов
| // Бинарные арифметические операторы | ||
|
|
||
| Phasor operator + (const Phasor& a, const Phasor& b) { | ||
| Phasor result = a; |
There was a problem hiding this comment.
тогда можно было принять аргумент a, сразу как копию
|
|
||
| // Конструктор от стека | ||
| Queue::Queue(const std::stack<int>& st) { | ||
| std::stack<int> temp = st; |
There was a problem hiding this comment.
можно тогда было срау принимать копированием
| std::vector<int> tempVec; | ||
|
|
||
| while (!temp.empty()) { | ||
| tempVec.push_back(temp.top()); |
There was a problem hiding this comment.
у нас же два вектора, можно было не создавать лишний вектор и сразу складывать элементы в нужный из двух
| } | ||
|
|
||
| // обеспечивает доступ на запись и чтение к элементу в конце очереди (неконстантная версия) | ||
| int& Queue::Back() { |
There was a problem hiding this comment.
не понятно почему Front() и Back() не имеют одинаокового подхода
| } | ||
| //копии для сравнения | ||
| Queue q1 = *this; | ||
| Queue q2 = other; |
There was a problem hiding this comment.
зачем создавать копии, мы сами пишем класс, и знаем его внутреннее устройство, количество элемментов, можно сделать сравнение оптимальным, без копирования
| size_t head_ = 0; | ||
| size_t tail_ = 0; | ||
| size_t size_ = 0; | ||
| bool full_ = false; |
There was a problem hiding this comment.
это лишнее поле, так как его легко получить сравнивая, например, size_ и capacity вектора
| std::vector<int> data_; | ||
| size_t head_ = 0; | ||
| size_t tail_ = 0; | ||
| size_t size_ = 0; |
There was a problem hiding this comment.
можно реализовать с двумя дополнительными полями
| if (capacity == 0) { | ||
| capacity = 1; | ||
| } | ||
| data_.resize(capacity); |
There was a problem hiding this comment.
поскольку мы находимся в конструкторе объекта, то в даннос случае лучше использовать reserve. Так как resize не только выделяет память, но и заполняет значениями по умолчанию, а далее мы заново перезаписываем значения с помощью Push
| void RingBuffer::Push(int value) { | ||
| if (full_) { | ||
| data_[tail_] = value; | ||
| tail_ = n_pos(tail_); |
There was a problem hiding this comment.
первые две строки можно выполнить безусловно, а если отказаться от full, то можно отказаться от else
| if (full_) { | ||
| return false; | ||
| } | ||
| Push(value); |
There was a problem hiding this comment.
дважды проверка на full_ одна в теле метода, другая при вызове Push, эффективнее было реализовать TryPush и через него реализовать Push
| return false; | ||
| } | ||
| value = data_[head_]; | ||
| Pop(); |
There was a problem hiding this comment.
дважды проверка на Empty
|
|
||
| int RingBuffer::operator[](size_t index) const { | ||
| if (index >= size_) { | ||
| throw std::out_of_range("Index out of range"); |
There was a problem hiding this comment.
в операторе не принято делать проверку и бросать исключение, обычно для этого используется метод at()
| // UB согласно условию | ||
| return data_[head_]; | ||
| } | ||
| return data_[head_]; |
There was a problem hiding this comment.
получается можно не делать проверку
| // Состояние буфера | ||
|
|
||
| bool RingBuffer::Empty() const noexcept { | ||
| return !full_ && (head_ == tail_); |
There was a problem hiding this comment.
как будто размер с 0 сравнивать эффективно
| } | ||
|
|
||
| bool RingBuffer::Full() const noexcept { | ||
| return full_; |
There was a problem hiding this comment.
как будто размер с cpacity сравнивать эффективно, и можно не хранить поле, тем более, если вспомнить про выравнивание, то мы помимо одного байта ещё имеем большое выравнивание для хранения поля
| bool Stack::Pop(){ | ||
| if (stack.empty()){ | ||
| return false; | ||
| } else{ |
There was a problem hiding this comment.
else создает лишнюю вложенность
| } | ||
| } | ||
|
|
||
| int& Stack::Top(){ |
There was a problem hiding this comment.
не хватает пробела перед {
| stack.clear(); | ||
| } | ||
|
|
||
| void Stack::Swap(Stack& stack2){ |
There was a problem hiding this comment.
other принято использовать в качестве имени
2 participants
No description provided.