-
Notifications
You must be signed in to change notification settings - Fork 33
Титаев Андрей #19
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?
Титаев Андрей #19
Conversation
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
|
|
||
| return static_cast<int64_t>(a) + static_cast<int64_t>(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.
Лишний каст, после приведения одной переменной, вторая приведется неявно, необходимо понимать и использовать данные возможности языка для лаконичности
| #include <cstddef> | ||
| #include <stdexcept> | ||
|
|
||
| size_t CharChanger(char* array, size_t size, char delimiter) { |
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.
Не нужно изменять сигнатуру функции если не просят. В данном случае это некорректное изменение, так как теряется часть функциональности
| // Приведения текущего символа к определенным типам: пробел, число или ASCII символ | ||
| bool isSpace = std::isspace(static_cast<unsigned char>(currentChar)) != 0; | ||
| bool isDigit = std::isdigit(static_cast<unsigned char>(currentChar)) != 0; | ||
| bool isAlpha = std::isalpha(static_cast<unsigned char>(currentChar)) != 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.
Зачем создавать переменные если переменная:
- Используется единожды
- Если используется, то другие не используются (на каждом шаге выполняются все проверки)
| // Подсчёт длины последовательности одинаковых символов | ||
| size_t nextPos = readPos + 1; | ||
| while (nextPos < size - 1 && array[nextPos] == currentChar) { | ||
| nextPos++; |
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 (isSpace) { | ||
| array[writePos++] = delimiter; |
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.
более выразительно переписать с использованием continue и следовательно убрать вложенность и непосредственно следом идущий else, так как иначе приходится листать весь блок кода else чтобы понять, будет ли ещё что-то изменяться
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| uint8_t value = static_cast<uint8_t>(flags); | ||
| uint8_t all_bits = static_cast<uint8_t>(CheckFlags::ALL); |
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; | ||
| } | ||
|
|
||
| const std::pair<CheckFlags, const char*> flag_names[] = { |
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.
Это могло бы быть уместно в случае статической переменной, но в данном случае при каждом вызове функции мы создаем массив из шести пар, которые можем даже не использовать, очень неэффективный поход.
|
|
||
|
|
||
| constexpr long double CM_TO_M = 0.01L; | ||
| constexpr long double M_TO_CM = 1.0L / CM_TO_M; |
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.
Как будто объявлено лишнее количество констант и удобнее объявить меньшее количество а часть выражений перенести в код операторов
| constexpr long double IN_TO_CM = IN_TO_M * 100.0L; | ||
| constexpr long double CM_TO_IN = 1.0L / IN_TO_CM; | ||
|
|
||
|
|
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.
лишняя строка
|
|
||
| void PrintBits(long long value, size_t bytes) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| void PrintBits(long long value, size_t bytes) { |
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.
лишний пробел перед void
|
|
||
| // Сдвиг, чтобы напечатать младшие байты | ||
| ULL mask = (bytes == BYTES_IN_LONG_LONG) | ||
| ? ~0ULL |
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.
Как-то неожиданно отступ стал 2 пробела вместо принятых 4. Обычно пишут в одном стиле, когда сами пишут
| } | ||
|
|
||
| if(a != 0) { // two roots | ||
| int discriminant = b * b - 4 * a * c; |
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.
то 2, то 4 пробела
|
|
||
| cout << setprecision(6); | ||
| if(discriminant > 0) { | ||
| double discriminantRoot = sqrt(discriminant); |
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::sqrt
| cout << setprecision(6); | ||
| if(discriminant > 0) { | ||
| double discriminantRoot = sqrt(discriminant); | ||
| double x1 = (-b - discriminantRoot) / (2.0*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.
пробелы вокруг *
| long double sumOfSquares = 0; | ||
|
|
||
| for(size_t i = 0; i < size; ++i) { | ||
| double &value = values[i]; |
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.
const double&
| double ApplyOperations(double a, double b /* other arguments */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| double ApplyOperations(double a, double b, double (**ops)(double, double), size_t size) { |
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.
Для наглядности хотелось, чтобы это был массив укзателей на функцию
| double sum = 0.0; | ||
| for (size_t i = 0; i < size; ++i) { | ||
| if (ops[i] == nullptr) { | ||
| continue; // íå âûïîëíÿòü îïåðàöèþ, åñëè óêàçàòåëü ôóíêöèè ðàâåí nullptr |
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.
Вероятно нужно поправить кодировку на UTF-8
| } | ||
| void PrintMemory(double value, bool reverse = false) { | ||
| using std::cout, std::hex, std::uppercase, std::setw, std::setfill; | ||
| const uint8_t* bytes = reinterpret_cast<const uint8_t*>(&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.
Большое дублирование кода после того как мы получили указатель и зная sizeof(value) далее можно было вызывать одну общую функцию, и переиспользовать код
| previousCharacter = currentCharacter; | ||
| } | ||
|
|
||
| // Ïðîâåðêà ïîñëåäíåãî ñåãìåíòà |
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 longestStringStart; | ||
| } | ||
|
|
||
| char* FindLongestSubsequence(char* begin, char* end, size_t& count) { |
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.
Хорошее применение
| } | ||
|
|
||
|
|
||
| // Ïå÷àòü ñ îãðàíè÷åíèÿìè |
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.
Кодировка
| ptr += reversed ? -1 : 1; | ||
| } | ||
|
|
||
| // Íå âñå ñèìâîëû ñìîãëè íàïå÷àòàòü íà îäíîé ñòðîêå, ïåðåíîñèì |
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.
Кодировка
| // Ïå÷àòü áåç îãðàíè÷åíèé ïî êîëè÷åñòâó ýëåìåíòîâ | ||
| cout << "["; | ||
| if (limit == 0 || limit >= count) { | ||
| if (reversed == 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.
!reversed - лучше подходит
| if (ptr != first) cout << ", "; | ||
| cout << *ptr; | ||
| } | ||
| } |
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.
тут практически полное дублирование, можно вынести в функцию, либо производить проверку reversed ? --ptr : ++ptr
|
|
||
| stats.avg = sum / data.size(); | ||
| double variance = (sum_of_squares / data.size()) - (stats.avg * stats.avg); | ||
| variance = variance >= 0.0 ? variance : 0.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.
лишнее присваивание variance = variance
| return lhs.course > rhs.course; | ||
| } | ||
|
|
||
| return lhs.birth_date < rhs.birth_date; |
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::tie , просто в операндах можно связывать как lhs так и rhs, чтобы выразить меньше и больше, а сам оператор в выражении return останется <
| if (!first) os << ", "; | ||
| os << "DEST"; | ||
| first = 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.
Много дублирования, тоже лучше подумать как от него избавиться, цикл или функция
| #include <vector> | ||
| #include <utility> | ||
|
|
||
| std::pair<std::vector<int>::const_iterator, std::vector<int>::const_iterator> MinMax(const std::vector<int>& vec) { |
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>::const_iterator
| ++it; | ||
| } | ||
|
|
||
| while ((it + 1) < vec.end()) { |
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.
лишние скобки для it + 1
| std::ostream& operator<<(std::ostream& os, const CircleRegionList& list) { | ||
| if (list.empty()) { | ||
| os << "{}"; | ||
| } 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.
лучше внутри if добавить return и не делать вложенность внутрь else. По возможности стараются избегать else
| count = (to - from + step - 1) / step; | ||
| } else { | ||
| count = (to - from + step + 1) / step; | ||
| } |
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> result; | ||
| result.reserve(count); | ||
|
|
||
| bool isForward = step > 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.
тем более тут все равно используется отдельная переменная для этого.
Но можно и без неё если используется в одном месте
| } | ||
| std::vector<int> Unique(const std::vector<int>& vec) { | ||
| if (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.
{} - более выразительно с учетом известного возвращаемого типа
| if (vec[i] != vec[i - 1]) { | ||
| ++unique_count; | ||
| } | ||
| } |
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.
Это может быть излишне, поскольку дважды проходим по всему вектору.
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.
@Andrey23525, код понравился, аккуратно, выразитльно, задачи решены достаточно оптимально, указал некоторые моменты
No description provided.