Шайхуллин Евгений #18
Conversation
01_week/tasks/addition/addition.cpp
Outdated
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| int64_t res = static_cast<int64_t>(a); // Присвоение и приведение типов. | ||
| res = res + static_cast<int64_t>(b); // Вычисление суммы и приведение типов. |
There was a problem hiding this comment.
В данном случае это лишнее приведение типов, поскольку тип int64_t res то сработает implicit cast b к данному типу
01_week/tasks/addition/addition.cpp
Outdated
| } | ||
| int64_t res = static_cast<int64_t>(a); // Присвоение и приведение типов. | ||
| res = res + static_cast<int64_t>(b); // Вычисление суммы и приведение типов. | ||
| return res; |
There was a problem hiding this comment.
Желательно написать вместо res сразу все выражение return a + b (с учетом каста одного из операндов), это будет нагляднее
01_week/tasks/rms/rms.cpp
Outdated
| double sum = 0.0; | ||
|
|
||
| if(0 == size) return 0.0;// | ||
| if(&values[0] == nullptr) return 0.0; |
There was a problem hiding this comment.
нет пробела после if
01_week/tasks/rms/rms.cpp
Outdated
| if(0 == size) return 0.0;// | ||
| if(&values[0] == nullptr) return 0.0; | ||
|
|
||
| for(size_t i = 0; i < size; i++){ |
There was a problem hiding this comment.
нет пробела после for и перед {
01_week/tasks/rms/rms.cpp
Outdated
| if(&values[0] == nullptr) return 0.0; | ||
|
|
||
| for(size_t i = 0; i < size; i++){ | ||
| sum += values[i]*values[i]; |
There was a problem hiding this comment.
нет пробелов вокруг оператора *
01_week/tasks/rms/rms.cpp
Outdated
| } | ||
| double sum = 0.0; | ||
|
|
||
| if(0 == size) return 0.0;// |
There was a problem hiding this comment.
Лучше использовать литерал для беззнаковых 0u
01_week/tasks/rms/rms.cpp
Outdated
| double CalculateRMS(double values[], size_t size) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| double sum = 0.0; |
There was a problem hiding this comment.
В данном случае либо sum создается рано, так как далее две короткие ветви выхода из функции, в которых sum не участвует и возвращается литерал, либо возвращать не литерал
01_week/tasks/rms/rms.cpp
Outdated
| if(0 == size) return 0.0;// | ||
| if(&values[0] == nullptr) return 0.0; | ||
|
|
||
| for(size_t i = 0; i < size; i++){ |
There was a problem hiding this comment.
Постфиксный инкремент создает копию, в данном случае сразу можно менять i
01_week/tasks/rms/rms.cpp
Outdated
| double sum = 0.0; | ||
|
|
||
| if(0 == size) return 0.0;// | ||
| if(&values[0] == nullptr) return 0.0; |
There was a problem hiding this comment.
Это некорректная проверка приводящая к UB, values это уже указатель, поэтому если он nullptr то values[0] разыменовывает нулевой указатель.
Если быбыли соответствующие проверки в тестах, то этот код бы упал
| float fA = static_cast<float>(a); // Явное приведение типа int к float | ||
| float fB = static_cast<float>(b); // Явное приведение типа int к float | ||
| float fC = static_cast<float>(c); // Явное приведение типа int к float | ||
| float disc = static_cast<float>(c); // Явное приведение типа int к float |
There was a problem hiding this comment.
Слишком рано определяется множество переменных, дальше идут короткие ветки выхода из функций и если они срабатывают, то бесполезно создается множество переменных с преобразованием во float, во-вторых желательно было их не создавать, а выполнить некоторые преобразования по месту в выражениях и довериться неявному приведению
| } | ||
|
|
||
| // Если x = 0 | ||
| if(((a == 0) && (c == 0)) || ((b == 0) && (c == 0))) |
There was a problem hiding this comment.
Есть излишнии скобки здесь и выше, труднее читать. в соответствии с приоритетом операторов скобки вокруг проверки на равенсво нулю можно опестить
|
|
||
| // Если a, b, c равны нулю – решений бесконечно много. | ||
| if((a == 0) && (b == 0) && (c == 0)){ | ||
| std::cout <<"infinite solutions"; |
There was a problem hiding this comment.
Вокруг операторов с двух сторон пробел по style guide
|
|
||
| if(a == 0) // Своего рода ускорение вычисления при a равном нулю | ||
| { | ||
| x1 = (-fC)/fB; |
There was a problem hiding this comment.
лишние скобки, нет пробелов
| } | ||
| else // Решение через дискрименант | ||
| { | ||
| disc = fB * fB - 4 * fA * fC; |
There was a problem hiding this comment.
Достаточно было сделать float disc, а лучше double и не заводить переменные fA, fB, fC
| // определяем вывод в зависимости от X1 и X2 | ||
| if (x1 == x2) std::cout << std::setprecision(6) << x1 ; | ||
| else if(x1 < x2) std::cout << std::setprecision(6) << x1 << ' ' << x2; | ||
| else std::cout << std::setprecision(6) << x2 << ' ' << x1; |
There was a problem hiding this comment.
визуально очень плохо выглядит, так не делают
| } | ||
|
|
||
| x1 = (negativeUnit * fB - sqrt(disc))/(2*fA); | ||
| x2 = (negativeUnit * fB + sqrt(disc))/(2*fA); |
There was a problem hiding this comment.
Абсолютно не понял назначение целой переменно negativeUnit если можно использовать унарный минус. Нет std:: у sqrt. Нет пробелов вокруг операторов
| // Константы для преобразования | ||
| #define IN_TO_CM 2.54 | ||
| #define FT_TO_IN 12.0 | ||
| #define M_TO_CM 100.0 |
There was a problem hiding this comment.
Лучше использовать constexpr переменные
| } | ||
|
|
||
| constexpr double operator"" _m_to_cm(long double x) { | ||
| return static_cast<double>( x * M_TO_CM); // |
There was a problem hiding this comment.
лишний пробел перед x
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Миллион лишних новых строк
| return static_cast<double>( x * FT_TO_IN); // преобразование фт в дюймы | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Если есть комментарий достаточно одной новой строки
| throw std::runtime_error{"Not implemented"}; | ||
|
|
||
| // Защита от неверного значения размера в байтах. Размер в байтах должен быть от 1 до 8. | ||
| if(bytes == 0) return; |
There was a problem hiding this comment.
отсутствие пробела и лишний пробел
| if(bytes == 0) return; | ||
| if(bytes > 8) return; | ||
|
|
||
| char buf[90] = {'\0'}; // Объявление и инициализация массива '\0'. |
There was a problem hiding this comment.
Это излишне и не универсально, в данной задаче можно было сразу выводить в поток
| { | ||
| if(sizeBit != count) | ||
| { | ||
| buf[countBuf + count] = 39; |
There was a problem hiding this comment.
это magic value - от этого в коде принято избавляться, для представления данного символа нужно использовать escape-последовательности
| tempBit = 0x01; | ||
| while(temp > countBit) | ||
| { | ||
| countBit ++; |
There was a problem hiding this comment.
Пробел не ставится, и постфиксный инкремент создает копию
| unsigned long long sizeBit = bytes*8; // количество бит для вывода | ||
| unsigned long long countFour = 0; | ||
| unsigned long long countBuf = 0; // счетчик для элементов массива | ||
| unsigned long long countBit = 0; |
There was a problem hiding this comment.
много лишних переменных и неоргонизованного кода, задача решается более компактно
| if(data > static_cast<uint8_t>(CheckFlags::ALL)) return; | ||
| if(data != 0) | ||
| { | ||
| if(data & static_cast<uint8_t>(CheckFlags::TIME)){ |
There was a problem hiding this comment.
двойной отступ во вложенном if
| void PrintCheckFlags(CheckFlags flags) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
|
|
||
| char buf[40] = {'\0'}; |
There was a problem hiding this comment.
Это неудобно, достаточно было выводить в поток сразу часть строки
| i++; | ||
| } | ||
|
|
||
| buf[i++] = 'T'; buf[i++] = 'I'; buf[i++] = 'M'; buf[i++] = 'E'; |
There was a problem hiding this comment.
Везде постфиксный инкремент плохо, написанно в одну строку плохо, дублирование всей логики и кода плохо
| #include <stdexcept> | ||
|
|
||
| // !!!! ВАЖНО !!!! Изначальное объявление функции изменено в части инициализации передаваемого элемента с «char delimiter = ' '» на «char delimiter». | ||
| size_t CharChanger(char array[], size_t size, char delimiter) { |
There was a problem hiding this comment.
Это некорректное исправление!
В тестах не было проверки на разделитель по умолчанию, но он должен быть задан в соответствии с сигнатурой функции по заданию
| { | ||
| symbol = array[i]; // Присваиваем значение элемента массива для анализа. | ||
|
|
||
| if(('0' <= symbol) && (symbol <= '9')) newSymbol = '*'; // Если символ - цифра меняем ее на ‘*’. |
There was a problem hiding this comment.
В стандартной библиотеке есть готовые функции для данных проверок, предпочтительно использовать их
| for (size_t i = 0; i < size; ++i) { // если более одной операции, проходим все. | ||
| // if(operations[i] == nullptr) return 0.0; // Если указатель на nullptr -ошибка возвращаем 0 | ||
| // total_sum += operations[i](a, b); // учет результата операции | ||
| if(operations[i] != nullptr) total_sum += operations[i](a, b); // учет результата операции |
There was a problem hiding this comment.
лишние пробелы, отсутствие пробела после if
| { | ||
| if(begin == nullptr) return end; // Проверка корректности адреса начала массива. | ||
| if(end == nullptr) return end; // Проверка корректности адреса конца массива. | ||
| if(begin >= end) return end; // Проверка корректности, адрес начала массива должен быть меньше чем адрес конца массива. |
There was a problem hiding this comment.
Условия необходимо объединить в одно, используя логические операторы, поскольку возвращаемое значение везде одно
|
|
||
| // const int* result = end; | ||
| const int* ptr = end; // Инициализируем адрес на проверяемый элемент массива. | ||
| ptr--; |
There was a problem hiding this comment.
постфиксный декремент
| // cout<< "\r\n value = "<< *ptr << " addres ptr = "<< ptr; | ||
| if(predicate(*ptr)) return ptr; | ||
|
|
||
| ptr --; |
| if(tempR < 10) buf[i*2] = tempR + 48; | ||
| else buf[i*2] = tempR - 10 + 'A'; | ||
| if(tempL < 10) buf[i*2+1] = tempL + 48; | ||
| else buf[i*2+1] = tempL - 10 + 'A'; |
03_week/tasks/filter/filter.cpp
Outdated
|
|
||
| // вычисление элементов контейнера. | ||
|
|
||
| if(temp != 0) |
There was a problem hiding this comment.
лучше инвертировать условие, чтобы не создаватьдополнительную вложенность
03_week/tasks/filter/filter.cpp
Outdated
|
|
||
|
|
||
| data.resize(j); | ||
| return ; |
03_week/tasks/filter/filter.cpp
Outdated
|
|
||
| data.resize(j); | ||
| return ; | ||
|
|
03_week/tasks/filter/filter.cpp
Outdated
| } | ||
| } | ||
|
|
||
|
|
03_week/tasks/find_all/find_all.cpp
Outdated
| /* return_type */ FindAll(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } | ||
| std::vector<size_t> FindAll(const std::vector<int>& data, bool(*op)(int)) |
03_week/tasks/minmax/minmax.cpp
Outdated
|
|
||
| /* return_type */ MinMax(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| std::pair<std::vector<int>::iterator, std::vector<int>::iterator> MinMax(std::vector<int>& data) { |
There was a problem hiding this comment.
Для длинных типов лучше ввести псевдоним, в данно случае можно для итераторов псевдоним добавить
Когда производится поиск миниму и максимума никогда не подразумевается возможность изменение контейнера, поэтому в качестве входного аргумента должен быть const контейнер
03_week/tasks/minmax/minmax.cpp
Outdated
| // проверка, что контейнер не пуст | ||
| if(data.size() == 0){ // контейнер пуст | ||
| // return {begin, begin}; // Используем uniform initialization | ||
| return std::pair<std::vector<int>::iterator, std::vector<int>::iterator>(begin, begin); |
03_week/tasks/minmax/minmax.cpp
Outdated
| } | ||
|
|
||
| // перегрузка функции | ||
| std::pair<std::vector<int>::const_iterator, std::vector<int>::const_iterator> MinMax(const std::vector<int>& data) { |
There was a problem hiding this comment.
Вроде нигде не нужно менять элемент по интератору в услвоию, поэтому две функции излишне
Поэтому нужно оставить одну. В любом случае необходимо избавляться от полного дублирования
| // Оператор вывода для CircleRegion | ||
| std::ostream& operator<<(std::ostream& os, const CircleRegion& сircleRegion) { | ||
| // работа с контейнером "pair" | ||
| // первый элемент типа Circle, второй bool |
There was a problem hiding this comment.
в целом такие комментарии излишние, поскольку CircleRegion это пара соответствующих занчений
03_week/tasks/range/range.cpp
Outdated
| throw std::runtime_error{"Not implemented"}; | ||
| std::vector<int> Range(int from, int to, int step = 1) { | ||
|
|
||
| if(from == to) return std::vector<int>(); // проверка равенства начала и конца интервала |
There was a problem hiding this comment.
{} - более короткий вариант пердпочтительнее
There was a problem hiding this comment.
@9617771614, основные проблемы
- Разыменование nullptr из-за некорректной проверки на nullptr (01_week)
- Использование постфиксного инкремента в циклах
- Излишние явные приведения типов, не используются неявные касты
- Создание избыточных переменных, неэффективный код
- Неаккуратный код: проблемы со стилем, лишние строки и пробелы, разный стиль
- Дублирование функциональности
Stack
Временно отключаю контроль задачь недели 3
Временно отключаю контроль задачь недели 2 иначе в проверке куча строк.
Временно отключаю контроль задачь недели 1 иначе в проверке куча строк.
2 participants
No description provided.