Conversation
| int64_t Addition(int a, int b) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| return (int64_t)a + b; |
There was a problem hiding this comment.
c-style cast не уместен, static_cast
| int count_repeat = 1; | ||
|
|
||
| std::string tmp; | ||
|
|
There was a problem hiding this comment.
зачем все эти пустые строки?
| if (array[i] == prev_ch) { | ||
| if (array[i] != ' ') { | ||
| ++count_repeat; | ||
| } |
There was a problem hiding this comment.
это достаточно короткая ветвь, поэтому добавить строку присвоения из конца и continue, неудобно листать большой блок кода ниже
| ++count_repeat; | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
и else убрать поскольку он станет лишним и уйдет уровень вложенности
| if (f(vec[i])) { | ||
| res.push_back(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
Два прохода, а если передана тяжелая функция предикат, не эффективно
| } | ||
| } | ||
|
|
||
| return std::make_pair(min, max); |
There was a problem hiding this comment.
примерно 3 лишних пустых строки
|
|
||
| auto MinMax(const std::vector<int>& vec) { | ||
| if (vec.empty()) { | ||
| return std::make_pair(vec.end(),vec.end()); |
There was a problem hiding this comment.
нет пробела после запятой
| res.push_back(vec[i + 1]); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
два прохода не нужны
18thday
left a comment
18thday
left a comment
There was a problem hiding this comment.
- не уместно используется C-style каст
- местами используется постфиксный декремент инкремент вместо префиксного
- разный стиль кода местами
- неуместные лишние пустые строки
- дублирование кода
- часто неэффективные решения
- const_cast UB
| } | ||
|
|
||
| m_data.pop_back(); | ||
|
|
There was a problem hiding this comment.
две лишние пустые строки
| } | ||
|
|
||
| const int& Stack::Top() const { | ||
| return (const_cast<Stack*>(this))->Top(); |
There was a problem hiding this comment.
UB const_cast
когда метод в одну строку, особого смысла нет вызывать метод из метода
| } | ||
|
|
||
| bool Stack::operator==(const Stack& other) const { | ||
| return this->m_data == other.m_data; |
There was a problem hiding this comment.
использование this-> лишнее, так не нужно писать
| size_t m_current_count; | ||
| }; | ||
|
|
||
| RingBuffer::RingBuffer(size_t capacity) { |
There was a problem hiding this comment.
это все должно быть в списке инициализации
| RingBuffer::RingBuffer(size_t capacity, int fill) : RingBuffer(capacity) { | ||
| std::fill(m_data.begin(), m_data.end(), fill); | ||
| m_end_index = m_data.size() - 1; | ||
| m_current_count = m_data.size(); |
There was a problem hiding this comment.
в данном случае это избыточный код, мы сначала создаем по умолчанию а потом ещё раз проходимся и заполняем, в данном случае тоже лучше использовать список инициализации, у вектора есть конструткор который аналогичные аргументы и заполняет контейнер одинаковыми элементами
|
|
||
| bool RingBuffer::TryPush(int element) { | ||
| if (!Full()) { | ||
| Push(element); |
There was a problem hiding this comment.
дважды проверка на !Full будет выполняться
| m_end_index = (m_end_index + 1) % Capacity(); | ||
|
|
||
| if (m_end_index == m_start_index) { | ||
| m_start_index = (m_start_index + 1) % Capacity(); |
There was a problem hiding this comment.
можно вынести в приватный метод если часто повторяется и принимать по ссылке idx
| bool RingBuffer::TryPop(int& element) { | ||
| if (!Empty()) { | ||
| element = m_data[m_start_index]; | ||
| Pop(); |
There was a problem hiding this comment.
дважды будет проверка !Empty
| } | ||
|
|
||
| const int& RingBuffer::operator[](size_t index) const { | ||
| return (const_cast<RingBuffer*>(this))->operator[](index); |
There was a problem hiding this comment.
UB const_cast, опять же, строка длиннее
|
|
||
| for (size_t i = 0; i < Size(); ++i) { | ||
| vec[i] = m_data[(m_start_index + i + skip) % Capacity()]; | ||
| } |
There was a problem hiding this comment.
если в коде выше добавить skip = 0 можно вынести цикл
| } | ||
| } | ||
|
|
||
| Queue::Queue(const std::vector<int>& vec) : Queue(vec.size()) { |
There was a problem hiding this comment.
можно сконструировать напрямую m_output
| } | ||
|
|
||
| Queue::Queue(std::initializer_list<int> il) : Queue(il.size()) { | ||
| std::reverse_copy(il.begin(), il.end(), std::back_inserter(m_output)); |
There was a problem hiding this comment.
можно сконструировать напрямую нужный вектор, без лишних действий
| if (m_output.empty()) { | ||
| std::reverse_copy(m_input.cbegin(), m_input.cend(), std::back_inserter(m_output)); | ||
| m_input.clear(); | ||
| } |
There was a problem hiding this comment.
это операция перекладыания элемментов может иметь понятное название и вынесена в отдельный приватный метод
| } | ||
|
|
||
| const int& Queue::Front() const { | ||
| return (const_cast<Queue*>(this))->Front(); |
There was a problem hiding this comment.
UB const_cast для константного объекта
| std::reverse_copy(other.m_output.cbegin(), other.m_output.cend(), std::back_inserter(value2)); | ||
| std::copy(other.m_input.cbegin(), other.m_input.cend(), std::back_inserter(value2)); | ||
|
|
||
| return value1 == value2; |
There was a problem hiding this comment.
не оптимальное сравнение, мы знаем внутреннее устройство и можем сравнить без копирования, в данном случае это плохое решение
| return Phasor(num, 0.0) / other; | ||
| } | ||
|
|
||
| Phasor Phasor::operator+=(const Phasor& other) { |
There was a problem hiding this comment.
почему возвращается не ссылка на текущий объект, а копия?
| double phase = std::atan2(m_imag, m_real); | ||
|
|
||
| if (std::fabs(phase - (-std::numbers::pi)) < 1e-9) { | ||
| return std::numbers::pi; |
There was a problem hiding this comment.
можно ввести using внутри класса, чтобы писать pi
| double Phasor::Phase() const { | ||
| double phase = std::atan2(m_imag, m_real); | ||
|
|
||
| if (std::fabs(phase - (-std::numbers::pi)) < 1e-9) { |
There was a problem hiding this comment.
пороговое значение лучше задать константой, чтобы можно было менять в одном месте
|
1 participant
No description provided.