-
Notifications
You must be signed in to change notification settings - Fork 33
Мельников Григорий #33
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?
Conversation
| int64_t a_64 = static_cast<int64_t>(a); | ||
| int64_t b_64 = static_cast<int64_t>(b); | ||
| int64_t sum = a_64 + b_64; | ||
| return sum; |
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. Причем достаточно бы было тогда одного static_cast, второй операнд не следует приводить явно, так как он бы преобразовался с помощью неявного каста. Принято использовать такую возможность и не писать явный каст самостоятельно второй раз
| if (islower(current)) { //проверка на пропись | ||
| array[position] = current - 32; | ||
| } | ||
| if (isupper(current)) { //проверка на пропись |
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 (!first) check_flags += ","; | ||
| check_flags += "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.
дублирование кода, много строк с одинаковыми действиями, можно было избавиться от них
| } | ||
| constexpr long double operator""_cm_to_m(long double cm) { | ||
| return cm/100.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.
Не принято использовать magic values в коде, пожалуй кроме 100, 1000 к примеру.
В данном случае корректнее добавить константы с понятными именами, например для 12 0.0254 0.3048 и уже в коде их использовать
|
|
||
| ++comma_position; | ||
| } | ||
| std::cout << bit_num; |
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.
судя по коду можно было писать сразу в поток без создания дополнительного массива
| @@ -0,0 +1,41 @@ | |||
| // ft -> в другие единицы | |||
| constexpr long double operator""_ft_to_in(long double ft) { | |||
| return ft*12.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.
не хватает пробелов вокруг операторов
| return; | ||
| } | ||
| //Проверяем дискрименант | ||
| double D = 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.
- чтобы избежать проблем с переполнением при умножении может быть лучше вычислять в double 4.0 и явный каст одного из
b - пробелы вокруг операторов ставятся
| std::cout << std::setprecision(6) << x1; | ||
| return; | ||
| } | ||
| if (D < 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.
лишнее условие, можно сразу выполнить код, а лучше поменять местами с D > 0, так как это более короткая ветвь
| } | ||
| //Проверяем дискрименант | ||
| double D = b*b-4*a*c; | ||
| if (D > 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.
нет пробела перед {
и лучше поменять местами с более короткой ветвью
| double CalculateRMS(double values[], size_t size) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| if (!values || size == 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.
пробел перед {
| for (size_t i = 0; i < size; ++i){ | ||
| sum_square += std::pow(values[i], 2); | ||
| } | ||
| double RMS = std::sqrt(sum_square/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.
- лишняя перемменная
- пробелы вокруг оператора
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| double ApplyOperations(double a, double b, double (*func[])(double, double), size_t size) { | ||
| if (size == 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.
пробел перед {
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| const int* FindLastElement(const int* begin, const int* end, bool (*predicate) (const int)) { | ||
| if ((end - begin) <= 0 || end == nullptr || begin == 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.
проверку указателей можно сокращать, используя неявное приведение к bool !end || !begin
| return end; | ||
| } | ||
| const int* last = end; | ||
| for (; begin < end; ++begin){ |
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::cout << std::hex << std::uppercase << std::setw(2) << std::setfill('0') << static_cast<int>(ptr_char[i]); | ||
| } | ||
| std::cout << std::endl; | ||
| } |
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 от типа, можно было вызвать функцию, содержащую все инструкции для вывода, тем самым избавиться от дублирования
| std::cout << std::hex << std::uppercase << std::setw(2) << std::setfill('0') << static_cast<int>(ptr_char[i]); | ||
| } | ||
| std::cout << std::endl; | ||
| } |
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.
выглядит как достаточно большое дублирование, можно было бы переписать так, чтобы не дублирвоать
| } | ||
|
|
||
| count = max_length; | ||
| return const_cast<char*>(longest_start); |
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.
Это UB. если изначально был указатель на константу, то снимать константность это UB.
Хорошим решением было бы две функции одна принимающая и возвращающая указатели на константы, а вторая без константности. Тогда из неконстантной версии можно было бы вызвать константную и после снять константность с помощью const_cast
| int dist = end - begin; | ||
| unsigned int count = 0; | ||
| if (begin == nullptr || end == nullptr || (dist*dist) == 0){ // Проверка на nullptr или пустой массив | ||
| numbers+="]"; |
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::string numbers = "["; | ||
| int dist = end - begin; | ||
| unsigned int count = 0; | ||
| if (begin == nullptr || end == nullptr || (dist*dist) == 0){ // Проверка на 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.
комментарий излишний, не совсем понятна цель dist возводить в квадрат
| while (true){ | ||
| int num = *begin; | ||
| std::string reverse_number = ""; | ||
| while (num / 10 != 0) { // // Конвертация int в str и запись в массив numbers |
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.
тут нет массива numbers, комментарий не соответствует действительности
| dist = end - begin; | ||
| } | ||
| numbers += "]"; | ||
| std::cout << numbers << std::endl; |
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.
можно было обойтись без лишних действий с добавлением и инверсией строки
| auto* temp = ptr1; | ||
| ptr1 = ptr2; | ||
| ptr2 = temp; | ||
| } |
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 N = data.size(); | ||
| for (size_t i = 0; i < N; ++i){ | ||
| sum_of_x += data[i]; | ||
| sum_of_squares_x += static_cast<double>(data[i])*data[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.
пробелы вокруг оператора
| if (student1.score != student2.score) return student1.score < student2.score; | ||
| if (student1.course != student2.course) return student1.course > student2.course; | ||
| if (student1.birth_date != student2.birth_date) return student1.birth_date < student2.birth_date; | ||
| return 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.
в данном случае можно было также использовать std::tie за счет расположения различных аргументов student1 и student2 в одном операнде, можно выразить как < так и >
std::tie(rhs.mark, lhs.score, ....) < std::tie(lhs.mark, rhs.score, ....)
- lhs rhs компактнее и более индормативно student1, student2, на крайний случай student_lhs или lhs_student
| /* return_type */ operator|(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| CheckFlags operator|(CheckFlags flags1, CheckFlags flags2) { | ||
| uint8_t all = 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.
думаю эта переменная излишня, можно выполнять данную операцию по месту
| CheckFlags operator|(CheckFlags flags1, CheckFlags flags2) { | ||
| uint8_t all = static_cast<uint8_t>(CheckFlags::ALL); | ||
| uint8_t value1 = static_cast<uint8_t>(flags1) & all; // Обрезаем все биты слева которые заходят за допустимый размер all | ||
| uint8_t value2 = static_cast<uint8_t>(flags2) & 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_type */ operator|(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| CheckFlags operator|(CheckFlags flags1, CheckFlags flags2) { |
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.
lhs, rhs - и компактней и понятней
| 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.
как будто однотипный код много раз повторяется
|
|
||
| /* return_type */ MinMax(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| 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::const_iterator, так как тип выглядит громоздко.
- пробел между
std::pairи<лишний
| int max = vec[0]; | ||
| auto min_it = vec.begin(); | ||
| auto max_it = vec.begin(); | ||
| for (auto it = vec.begin(); it!= vec.end(); ++it){ |
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.
пробелы перед {, здесь и далее
| throw std::runtime_error{"Not implemented"}; | ||
| std::vector<int> Range(int from, int to, int step = 1) { | ||
| std::vector<int> result; | ||
| if ((to - from)*step <= 0) return result; // проверка на невалидные параметры |
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.
пробелы вокруг операторов
| ++write_index; | ||
| } | ||
| } | ||
| ++write_index; // Костыль чтобы учесть первый добавленный элемент |
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.
это локальная переменная, зачем?
| for (size_t i = 1; i < vec.size(); ++i) { | ||
| if (result[write_index] != vec[i]){ // если следующий элемент не равен предыдущему то добавляем его в result | ||
| result.push_back(vec[i]); | ||
| ++write_index; |
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.
может тогда стоило в условии result[write_index++]
18thday
left a comment
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.
@MelnikovGYu основные моменты:
- magic values
- UB const_cast
- дублирование кода, которое можно избежать
| for (size_t i = 0; i < stack1.size(); ++i){ | ||
| if (stack1[i] != stack2[i]) return false; | ||
| } | ||
| return true; |
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.
@MelnikovGYu лишнее копирование в операторе равенства Stack, мало заданий
No description provided.