Рычков Григорий #42
Рычков Григорий #42
Conversation
|
|
||
| DataStats CalculateDataStats(const std::vector<int>& data) { | ||
| DataStats result; | ||
| size_t n = data.size(); |
There was a problem hiding this comment.
лучше не использовать переменные из одного символа
| sum += value; | ||
| } | ||
|
|
||
| // ��������� ������� �������� |
There was a problem hiding this comment.
кодировка поехала, UTF-8 у файла должно быть
|
|
||
| Date() : year(0), month(0), day(0) {} | ||
|
|
||
| Date(unsigned y, unsigned m, unsigned d) : year(y), month(m), day(d) {} |
There was a problem hiding this comment.
- лишний пробел
- конструкторы лишние, достаточно было дать полям значения по умолчанию и компилятор бы справился с тривиальными конструкторами, не пришлось бы писать лишний код. обычно для структур так и делают
| return !(lhs < rhs); | ||
| } | ||
|
|
||
| int mark_priority(char mark) { |
There was a problem hiding this comment.
Это дишняя функция, символы имеет целочисленное значение, и их можно сравнивать. Функцию придется менять в случае добавления оценки, например F или Y или ещё какого-то статуса, поэтому лучше так не делать а сравнивать символы напрямую, тогда при измененнии возможных значений логика не сломается и изменения не нужны будут
| case 'B': return 4; | ||
| case 'C': return 3; | ||
| case 'D': return 2; | ||
| case 'Z': return 1; |
There was a problem hiding this comment.
тем более за счет того, что есть не последовательные оценки, компилятор может это не соптимизировать до работы с jump table и будут выполнятся проверки каждый раз, а можно просто сравнить символы
| return lhs.course > rhs.course; | ||
| } | ||
|
|
||
| return lhs.birth_date < rhs.birth_date; |
There was a problem hiding this comment.
этот оператор тоже пишется в одну строку с помощью std::tie, если поменять местави одно из полей rhs и lhs местами, то в каком операнде операции < они находятся, знак сравнения изменится на противоположный, таким образом когда упорядоченность можно выразить через < > без дополнительных особых условий, пользуются std::tie
| ALL = TIME | DATE | USER | CERT | KEYS | DEST | ||
| }; | ||
|
|
||
| constexpr uint8_t MASK = 0x3F; |
There was a problem hiding this comment.
не очень хороший вариант, поскольку есть ALL и значение 0x3F корректнее заменить с егоиспользованием или по месту использовать без переменной
| if ((flags & CheckFlags::DEST) != CheckFlags::NONE) { | ||
| if (!first) os << ", "; | ||
| os << "DEST"; | ||
| } |
There was a problem hiding this comment.
А тут почему с лямбда не сложилось как в предыдущем, или с отдельной функцией, много дублирования
| #include <vector> | ||
| #include <utility> | ||
|
|
||
| std::pair<std::vector<int>::const_iterator, std::vector<int>::const_iterator> |
There was a problem hiding this comment.
длинный тип, лучшебыло ввести для итераторов псевдонимы и их использовать
| } | ||
|
|
||
| std::pair<std::vector<int>::iterator, std::vector<int>::iterator> | ||
| MinMax(std::vector<int>& vec) { |
There was a problem hiding this comment.
- это лишняя функция, так как по условию не было требования возожности изменения по итератору значений Min Max
- передача по обычной ссылке без константности вводит в заблуждение, говоря пользователю, что вектор может поменяться внутри, но это не так
| } | ||
| } | ||
|
|
||
| return {min_it, max_it}; |
There was a problem hiding this comment.
к тому же тут полное дублирование,
| n = (to - from - 1) / step + 1; | ||
| } else { | ||
| n = (to - from + 1) / step + 1; | ||
| } |
There was a problem hiding this comment.
лучше инициализировать сразу переменную n, используя тернарный оператор при -1 +1
| result.reserve(vec.size()); | ||
|
|
||
|
|
||
| result.push_back(vec[0]); |
There was a problem hiding this comment.
почему не сделать цикл с 0?
| #include <vector> | ||
|
|
||
| std::vector<int> Range(int from, int to, int step) { | ||
| if (step == 0) { |
|
|
||
| result.reserve(vec.size()); | ||
|
|
||
|
|
There was a problem hiding this comment.
лишние пустые строки
| double mag = Magnitude(); | ||
| double phase = Phase(); | ||
| real_ = mag * std::cos(phase); | ||
| imag_ = mag * std::sin(phase); |
There was a problem hiding this comment.
не понял зачем тут тело цикла?
| Phasor Inv() const { | ||
| double mag = Magnitude(); | ||
| if (mag == 0.0) { | ||
| // �������� � �������� ������ �� ���������, ������ ���� |
| }; | ||
|
|
||
| Phasor operator+(const Phasor& lhs, const Phasor& rhs) { | ||
| Phasor result = lhs; |
There was a problem hiding this comment.
можно тогда было сразу принимать копию
| } | ||
| } | ||
|
|
||
| Queue(std::initializer_list<int> il) : in_(il) {} |
There was a problem hiding this comment.
два раза копируем в il и в in
| int& Front() { | ||
| if (Empty()) { | ||
| static int dummy; | ||
| return dummy; |
| } | ||
|
|
||
| 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; | ||
| size_t capacity_ = 0; |
There was a problem hiding this comment.
есть лишние поля, можно было обойтись двумя, поддерживать корректный capacity у вектора и использовать его метод capacity, из оставшихся трех полей, зная два можно найти 3
| explicit RingBuffer(size_t capacity) { | ||
| ensure_nonzero_capacity(capacity); | ||
| capacity_ = capacity; | ||
| buffer_.resize(capacity_); |
There was a problem hiding this comment.
это лишнее, в данном случае лучше воспользоваться списком инициализации, а корректное значение capacity можно получить использовав тернарный оператор, а ещё лучше std::max(1, capacity)
| buffer_.resize(capacity_); | ||
| } | ||
|
|
||
| RingBuffer(size_t capacity, int initial_value) : RingBuffer(capacity) { |
There was a problem hiding this comment.
у вектора есть конструктор от вместимости и значения, поэтому в данном случае также предпочтительнее список инициализации
| std::fill(buffer_.begin(), buffer_.end(), initial_value); | ||
| } | ||
|
|
||
| RingBuffer(std::initializer_list<int> init) { |
There was a problem hiding this comment.
В данном случае аналогично у вектора есть конструктор от std::initializer_list
| void Push(int value) { | ||
| if (Full()) { | ||
| buffer_[tail_] = value; | ||
| advance(tail_); |
There was a problem hiding this comment.
дублирование кода, общие части для двух ветвей
| if (Full()) { | ||
| return false; | ||
| } | ||
| Push(value); |
There was a problem hiding this comment.
получается в TryPush дважды проверка Full, непосредственно в теле метода и внутри Push, не эффективно
| void Pop() { | ||
| if (!Empty()) { | ||
| advance(head_); | ||
| --size_; |
There was a problem hiding this comment.
лучше сделать короткую ветвеь с return для пустого буфера
| bool operator!=(const Stack& other) const; | ||
|
|
||
| private: | ||
| std::vector<int> data_;}; |
There was a problem hiding this comment.
}; - на новой строке
18thday
left a comment
18thday
left a comment
There was a problem hiding this comment.
@Rygrotten неплохой код, но не все задачи решены по 1-3, отметил следующие моменты
- дублирование кода
- неоптимальное сравнение Queue с двойным копированием
- не используются списки инициализации (лишний код в теле конструкторов)
- лишние поля в RingBuffer
2 participants
No description provided.