-
Notifications
You must be signed in to change notification settings - Fork 33
Петров Николай #37
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?
Петров Николай #37
Conversation
03_week/tasks/unique/unique.cpp
Outdated
| } | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return {}; тип возвращаемого значения написан, поэтому не нужно его писать заново
|
|
||
| // Бинарные арифметические операторы | ||
|
|
||
| Phasor operator + (const Phasor& a, const Phasor& b) { |
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.
обычно написание операторов соответствует написанию функций, без пробелов
| // Бинарные арифметические операторы | ||
|
|
||
| Phasor operator + (const Phasor& a, const Phasor& b) { | ||
| Phasor result = a; |
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.
тогда можно было принять аргумент a, сразу как копию
|
|
||
| // Конструктор от стека | ||
| Queue::Queue(const std::stack<int>& st) { | ||
| std::stack<int> temp = st; |
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> tempVec; | ||
|
|
||
| while (!temp.empty()) { | ||
| tempVec.push_back(temp.top()); |
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.
у нас же два вектора, можно было не создавать лишний вектор и сразу складывать элементы в нужный из двух
| } | ||
|
|
||
| // обеспечивает доступ на запись и чтение к элементу в конце очереди (неконстантная версия) | ||
| int& Queue::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.
не понятно почему Front() и Back() не имеют одинаокового подхода
| } | ||
| //копии для сравнения | ||
| Queue q1 = *this; | ||
| Queue q2 = other; |
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_t head_ = 0; | ||
| size_t tail_ = 0; | ||
| size_t size_ = 0; | ||
| bool full_ = 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.
это лишнее поле, так как его легко получить сравнивая, например, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
можно реализовать с двумя дополнительными полями
| if (capacity == 0) { | ||
| capacity = 1; | ||
| } | ||
| data_.resize(capacity); |
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.
поскольку мы находимся в конструкторе объекта, то в даннос случае лучше использовать reserve. Так как resize не только выделяет память, но и заполняет значениями по умолчанию, а далее мы заново перезаписываем значения с помощью Push
| void RingBuffer::Push(int value) { | ||
| if (full_) { | ||
| data_[tail_] = value; | ||
| tail_ = n_pos(tail_); |
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.
первые две строки можно выполнить безусловно, а если отказаться от full, то можно отказаться от else
| if (full_) { | ||
| return false; | ||
| } | ||
| Push(value); |
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.
дважды проверка на full_ одна в теле метода, другая при вызове Push, эффективнее было реализовать TryPush и через него реализовать Push
| return false; | ||
| } | ||
| value = data_[head_]; | ||
| Pop(); |
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.
дважды проверка на 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
в операторе не принято делать проверку и бросать исключение, обычно для этого используется метод at()
| // UB согласно условию | ||
| return data_[head_]; | ||
| } | ||
| return data_[head_]; |
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 RingBuffer::Empty() const noexcept { | ||
| return !full_ && (head_ == tail_); |
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.
как будто размер с 0 сравнивать эффективно
| } | ||
|
|
||
| bool RingBuffer::Full() const noexcept { | ||
| return full_; |
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.
как будто размер с cpacity сравнивать эффективно, и можно не хранить поле, тем более, если вспомнить про выравнивание, то мы помимо одного байта ещё имеем большое выравнивание для хранения поля
04_week/tasks/stack/stack.cpp
Outdated
| bool Stack::Pop(){ | ||
| if (stack.empty()){ | ||
| return false; | ||
| } 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 создает лишнюю вложенность
04_week/tasks/stack/stack.cpp
Outdated
| } | ||
| } | ||
|
|
||
| int& Stack::Top(){ |
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.
не хватает пробела перед {
04_week/tasks/stack/stack.cpp
Outdated
| stack.clear(); | ||
| } | ||
|
|
||
| void Stack::Swap(Stack& stack2){ |
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.
other принято использовать в качестве имени
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.
@TookTM отметил следующие моменты:
- постфиксный инкремент использует вместо префиксного
- дублирование кода
- лишняя вложенность условий
- неоптимальное сравнение Queue с двойным копированием
- лишние поля в RingBuffer
No description provided.