Кузнецов Владимир #14
Conversation
…big task, add logest task, add pretty_array task
I want to save a local changes. ^T
|
|
||
| for (; it != end - 1; ++it){ | ||
| if (*it == *(it + 1)) ++curr_count; | ||
| else { |
There was a problem hiding this comment.
лучше выше добавить continue под if и не делать дополнительный уровень вложенности из-за else для кода ниже
| throw std::runtime_error{"Not implemented"}; | ||
| void PrintArray(const int* begin, const int* end, size_t limit = 0) { | ||
| auto _start = const_cast<int*>(begin); | ||
| auto _end = const_cast<int*>(end); |
|
|
||
| for (size_t i = 0; i < v_size; ++i) { | ||
| sum_sq += static_cast<long long>(vec[i]) * | ||
| static_cast<long long>(vec[i]); |
There was a problem hiding this comment.
достаточно одного вместо двух и лучше писать в одной строке
| if (date1.day > date2.day) return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
в данной задаче везеде можно использовать std::tie от нескольких полей, что позволяет запистаь условия в одну строку и так правильнее делать
| bool operator!=(const Date& date1, const Date& date2) { | ||
| return date1.year != date2.year || | ||
| date1.month != date2.month || | ||
| date1.day != date2.day; |
There was a problem hiding this comment.
можно легко вырзаить через предыдущий определенный оператор
| if (date1.day < date2.day) return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
как правило определяют оператор < и ==, а все остальные выражают через данные операторы
| } | ||
| if ((static_cast<CheckFlags>(res) == CheckFlags::NONE) || (!flag1 && !flag2)) { | ||
| return false; | ||
| } |
| if (func(*it)) { | ||
| auto temp = *it; | ||
| *it = *last_correct_it; | ||
| *last_correct_it = temp; |
There was a problem hiding this comment.
тут видимо надо было std::swap применить, либо перезаписывать непосредственно
| #include <vector> | ||
|
|
||
| typedef std::vector<int> vec_i_t; | ||
| typedef std::vector<int>::const_iterator cit_t; |
There was a problem hiding this comment.
для C++ предпочтительнее использовать using
| #include <stdexcept> | ||
| #include <vector> | ||
|
|
||
| typedef std::vector<int> vec_i_t; |
| std::vector<int> Range(int from, int to, int step = 1) { | ||
| if ((to == from) || | ||
| ((to > from) && (step <= 0)) || | ||
| ((from > to) && (step >= 0))) |
There was a problem hiding this comment.
можно было проверить шаг и по резульату проверки вернуть из тернарного оператора нужный результат сравнения to from
| } | ||
|
|
||
| Phasor& Phasor::operator-=(const Phasor& other) { | ||
| double real = this->Real() - other.Real(); |
There was a problem hiding this comment.
this-> не следует использовать внутри класса, это лишнее
| Queue::Queue(const std::vector<int>& input_vec) : m_in_vec(input_vec) {}; | ||
|
|
||
| Queue::Queue(const std::initializer_list<int>& input_list) | ||
| : m_in_vec(static_cast<std::vector<int>>(input_list)) { |
| m_out_vec.pop_back(); | ||
| return true; | ||
| } | ||
| else { |
There was a problem hiding this comment.
это лишняя вложенность в ветви выше уже есть return
| } | ||
| else { | ||
| SwapVecs(other.m_out_vec, m_out_vec, min_size_out);; | ||
| } |
There was a problem hiding this comment.
нужно было использовать стандартный swap, который принимает вектора
| if (this == &other) return true; | ||
|
|
||
| std::vector<int> queue_this(m_in_vec.size() + m_out_vec.size()); | ||
| std::vector<int> queue_other(other.m_in_vec.size() + other.m_out_vec.size()); |
There was a problem hiding this comment.
это лишнее, зачем создавать копии, если мы знаем устройство контейнера, и можем обойти элементы не прибегая к копированию
| std::vector<int> queue_this(m_in_vec.size() + m_out_vec.size()); | ||
| std::vector<int> queue_other(other.m_in_vec.size() + other.m_out_vec.size()); | ||
|
|
||
| if (queue_this.size() != queue_other.size()) return false; |
There was a problem hiding this comment.
эта проверка должна быть до строки выше, иначе зря скопированы были огромные вектора, если у них размеры разные
| return true; | ||
| } | ||
|
|
||
| bool Queue::operator!=(const Queue& other) const { |
There was a problem hiding this comment.
выражается через функцию выше, код не дублируется
| bool operator!=(const Stack& other) const; | ||
|
|
||
| private: | ||
| friend void SwapVecs(std::vector<int>& v1, std::vector<int>& v2, size_t min_size); |
| int trash = std::rand(); | ||
| int& ref_trash = trash; | ||
| return ref_trash; | ||
| } |
|
|
||
| void Stack::Clear() { | ||
| stack.resize(0); | ||
| stack.shrink_to_fit(); |
There was a problem hiding this comment.
не совсем корректно, достаточно вызвать clear() обычно clear не меняет размер контейнера а просто передвигает укзаатель конца на начало и позволяет снова записывать элементы, в общем функция должна работать за O(1)
| } | ||
| else { | ||
| SwapVecs(other.stack, stack, min_size); | ||
| } |
There was a problem hiding this comment.
просто надо воспользоваться стандартным swap для вектора
| size_t d2first = static_cast<size_t>(buf.m_first_ind - buf.m_buffer.begin()); | ||
| m_first_ind = m_buffer.begin() + d2first; | ||
| m_last_ind = m_buffer.begin() + d2last; | ||
| }; |
There was a problem hiding this comment.
сложно, можно было все сделать в списках инициализации
| int& RingBuffer::operator[](const size_t& ind) { | ||
| if (m_size == 0 || ind >= m_size || ind >= m_buffer.size()) { | ||
| int* n = nullptr; | ||
| return *n; |
There was a problem hiding this comment.
разыменование nullptr это UB
There was a problem hiding this comment.
Вызов Front и Back от пустой очереди не ограничивается и ведет к UB. Так написано в ТЗ
| if (m_size < m_buffer.capacity()) { | ||
| if (m_last_ind + 1 == m_buffer.end()) { | ||
| if (m_buffer.size() != m_buffer.capacity()) { | ||
| m_buffer.push_back(num); |
There was a problem hiding this comment.
слишком большая вложенность неудобно читать
| ++m_first_ind; | ||
| if (m_first_ind == m_buffer.end()) m_first_ind = m_buffer.begin(); | ||
| } | ||
| } |
There was a problem hiding this comment.
метод чрезвычайно слишком переусложнен, надо задуматься о дизайне класса тогда, можно и за 5 строк реализовать операцию
|
|
||
| bool RingBuffer::TryPush(const int& num) { | ||
| if (m_size == m_buffer.capacity()) return false; | ||
| this->Push(num); |
There was a problem hiding this comment.
this-> не используется внутри класса
| return true; | ||
| } | ||
|
|
||
| void RingBuffer::Resize(int new_capacity) { |
| res.push_back(*(it + i)); | ||
| } | ||
| return res; | ||
| } No newline at end of file |
There was a problem hiding this comment.
не понравилась реализация, можно было проще и лучше
18thday
left a comment
18thday
left a comment
There was a problem hiding this comment.
@vova22013 нужно много поработать над написанием кода:
- много лишних static_cast
- лишние переменные в функциях
- лишние тернарные операторы возвращающие true false
- UB const_cast
- некорректные сигнатуры функций
- много дублирования и лишнего в коде
- неоптимальные функции
- лишнее копирвоание при сравнение Queue
- использование this-> внутри класса
- лишний код
- не использование стандартных готовых функций
- не понравилась реализация RingBuffer
2 participants
No description provided.