Плотников Денис #20
Плотников Денис #20
Conversation
|
|
||
| int64_t Addition(int a, int b) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| return static_cast<int64_t>(a) + static_cast<int64_t>(b); |
There was a problem hiding this comment.
Лишний каст, второй операнд кастовать не обязателен, так как произойдет неявный каст к int64_t, и принято этим пользоваться, оставляя код чище
| } | ||
|
|
||
| size_t src {}; // индекс чтения | ||
| size_t dst {}; // индекс записи |
There was a problem hiding this comment.
пробел обычо не ставят перед {} в данном случае
| size_t dst {}; // индекс записи | ||
|
|
||
| while (array[src] != '\0' && src < size) { | ||
| char c { array[src++] }; // текущий символ |
There was a problem hiding this comment.
аналогично предыдущему и для переменных и списков инийиализации обычно пробелами не обрамляют фигурные скобки с внутренней стороны
| // Подсчет количества одинаковых символов | ||
| size_t count {1}; | ||
| while (array[src] != '\0' && src < size) { | ||
| const char next { array[src] }; |
There was a problem hiding this comment.
лучше не именовать переменную используемую в единственном месте, а использовать по месту, тем более на следующем цикле она уже не next, а next_next
| array[dst++] = '0'; | ||
| } else { | ||
| array[dst++] = static_cast<char>('0' + count); | ||
| } |
There was a problem hiding this comment.
в данном случае просится тернарный оператор в одну строку
| c = '*'; | ||
| } else if (std::islower(static_cast<unsigned char>(c))) { | ||
| c = std::toupper(static_cast<unsigned char>(c)); | ||
| } else if (std::isupper(static_cast<unsigned char>(c))) { |
There was a problem hiding this comment.
как будто лучше вместо else опставить условие !std::isupper(), а данную проверку убрать
| case 3: out += "CERT"; break; | ||
| case 4: out += "KEYS"; break; | ||
| case 5: out += "DEST"; break; | ||
| } |
There was a problem hiding this comment.
Это круто, понравилось решение, единственное нет отступа у case
| void PrintCheckFlags(CheckFlags flags) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| void PrintCheckFlags(CheckFlags flags) { | ||
| constexpr static const auto all { static_cast<uint8_t>(CheckFlags::ALL) }; |
There was a problem hiding this comment.
Считаю это излишним, так как применяется в единственном месте, и переменная мало весит, чтобы её делать static, не уверен что будет эффекктивнее чем ходить недалеко по стеку, вероятно лучше по месту применить каст
|
|
||
| std::string out {'['}; | ||
| bool first { true }; | ||
| for (uint8_t i = 0; std::cmp_less(i, std::popcount(all)); ++i) { |
There was a problem hiding this comment.
на каждом цикле считать std::popcount(all) не целесообразно
There was a problem hiding this comment.
Если all помечено constexpr и std::popcount --- тоже constexpr, то разве будет выполняться пересчёт на каждой итерации?
В таком случае и статичность all кажется вполне уместной.
| // Метры → дюймы | ||
| constexpr double operator""_m_to_in(long double v) { | ||
| return static_cast<double>(v / 0.0254); | ||
| } No newline at end of file |
There was a problem hiding this comment.
лучше в коде избегать magic value (за исключением 100 наверное), неплохо бы завести безымянный namespace с понятными константами 2.54, 0.3048, 12 и их использовать
|
|
||
| void PrintBits(long long value, size_t bytes) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| using value_t = decltype(value); |
There was a problem hiding this comment.
для использования в одном месте немного сомнительно
|
|
||
| std::string out { "0b" }; | ||
| for (size_t i = 0; i < bits; ++i) { | ||
| const value_t mask { (static_cast<value_t>(1) << (bits - 1 - i)) }; |
There was a problem hiding this comment.
можно использовать литерал для long long будет более выразительно нагромождения с псевдонимом
|
|
||
| if (num < total) { | ||
| if (limit != NO_LIMIT && num % limit == 0) | ||
| { |
| auto PrintElement = [&](const size_t idx, const int* const p) { | ||
| std::cout << *p; | ||
|
|
||
| const auto num { idx + 1 }; |
There was a problem hiding this comment.
может тогда имеет смысл передавать idx_next? idx не используется и зачем-то создается копия элемента увеличенная на 1
| for (size_t i = 0; p < cend; ++p, ++i) { | ||
| PrintElement(i, p); | ||
| } | ||
| } |
There was a problem hiding this comment.
применение лямбды сомнительно с учетом того что код дублируется, как будто необходимо просто имзенять --p ++p в тернарном операторе и использовать один цикл и не использовать лямбду с захватом вовсе, так как кода мало и используется в одном месте
| bestLen = currLen; | ||
| bestStart = currStart; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Несколько сомнительно использовать лямбду в коде
| #include <cstddef> | ||
|
|
||
| static constexpr size_t NO_LIMIT = 0; | ||
| static constexpr const size_t LIMIT_DEAFULT {NO_LIMIT}; |
| void SwapPtr(/* write arguments here */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| template <typename T> | ||
| void SwapPtr(T*& pp1, T*& pp2) { |
There was a problem hiding this comment.
это все-таки ссылка на указатель а не указатель на указатель
|
|
||
| const size_t n { data.size() }; | ||
|
|
||
| //? Математически корректно использовать n-1 для несмещённой оценки дисперсии выборки, |
There was a problem hiding this comment.
Дело не в корректности, а в наличие смещенной и несмещенной оценок. По умолчанию в данном случае подразумевается смещенная, так как не требует какой-то доп. квалификации и непонимания почему тесты не проходят
| if (birth_date != other.birth_date) | ||
| return birth_date < other.birth_date; | ||
|
|
||
| return false; |
There was a problem hiding this comment.
таким образом можно все свести к одной строке используя std::tie
|
|
||
| /* return_type */ operator^(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| auto operator&(const CheckFlags lhs, const CheckFlags rhs) { |
There was a problem hiding this comment.
Почему тогда не принимать неконстантную копию? и далее не плодить ещё переменных
| if (circle_region.second) { | ||
| // внутренняя область | ||
| os << "+"; | ||
| } else { |
| } else { | ||
| // внешняя область | ||
| os << "-"; | ||
| } |
| if (step == 0 || from == to || | ||
| (step > 0 && from > to) || | ||
| (step < 0 && from < to)) | ||
| { |
| const size_t size { | ||
| (to - from) % step == 0 | ||
| ? static_cast<size_t>((to - from) / step) | ||
| : static_cast<size_t>((to - from) / step) + 1 |
There was a problem hiding this comment.
судя по выражению тернарный оператор можно использовать только для + 1
| } | ||
|
|
||
| class Phasor { | ||
| inline static constexpr const double pi { std::numbers::pi }; |
There was a problem hiding this comment.
зачем это здесь если можно определить using для std::numbers::pi ?
| static constexpr const double EPSILON { 1e-12 }; // std::numeric_limits<double>::epsilon() | ||
|
|
||
| static bool Eq(double a, double b) { | ||
| static constexpr const double eps { EPSILON }; |
There was a problem hiding this comment.
зачем данное переопределение?
|
|
||
| static bool Eq(double a, double b) { | ||
| static constexpr const double eps { EPSILON }; | ||
| const std::array<double, 3> vals { 1.0, std::abs(a), std::abs(b) }; |
There was a problem hiding this comment.
Зачем ещё раз хранить на стеке переменные?
| input_.push_back(s.top()); | ||
| s.pop(); | ||
| } | ||
| std::reverse(input_.begin(), input_.end()); |
There was a problem hiding this comment.
почему сразу не складывать в output_?
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
не аккуратно, лишние пробелы перед, так неудобно читать код, лайк за отсутствие копирования
| bool Empty() const noexcept { | ||
| return Size() == 0; | ||
| } | ||
| auto Size() const noexcept -> std::size_t { |
There was a problem hiding this comment.
зачем тогда писать хинт -> std::size_t и писать auto, излишнее дублирование
| auto Front() -> int&; | ||
| auto Front() const -> const int&; | ||
| auto Back() -> int&; | ||
| auto Back() const -> const int&; |
There was a problem hiding this comment.
визуально очень плохо, пробелы не нужны
| } | ||
| auto Queue::Front() const -> const int& { | ||
| return FrontImpl(*this); | ||
| } |
There was a problem hiding this comment.
этоочень сомнительная конструкция для таких целей
| if (Full()) { | ||
| return false; | ||
| } | ||
| Push(value); |
There was a problem hiding this comment.
получается двойная проверка на Full
| return false; | ||
| } | ||
| value = Back(); | ||
| Pop(); |
There was a problem hiding this comment.
двойная проверка на Empty и !Empty
| } | ||
| auto Stack::Top() const noexcept -> const int& { | ||
| return TopImpl(*this); | ||
| } |
There was a problem hiding this comment.
зачем писать дополнительные 4 строки с шаблоном?
| /// @} | ||
|
|
||
| auto Top() noexcept -> int&; | ||
| auto Top() const noexcept -> const int&; |
There was a problem hiding this comment.
с хинтами менее удобно и наглядно
2 participants
No description provided.