-
Notifications
You must be signed in to change notification settings - Fork 33
Мананов Данил #21
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?
Мананов Данил #21
Conversation
This reverts commit 5ba2b46.
| unsigned char currentByte = bytes[i]; | ||
| std::cout << hexChars[currentByte >> 4]; | ||
| std::cout << hexChars[currentByte & 0x0F]; | ||
| } |
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.
поскольку тело цикла не меняется, можно было подумать как переписать условие и где использовать тернарный оператор чтобы не копипастить всеь код, а если бы тело было из 10 строк, 20?
|
|
||
| void PrintMemory(double number, bool reverseFlag = false) | ||
| { | ||
| const unsigned char* bytes = reinterpret_cast<const unsigned char*>(&number); |
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(currentLength > maxLength){ |
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(currentLength > maxLength){ | ||
| maxLength = currentLength; |
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 currentLength = 1; | ||
|
|
||
| for (const char* ptr = begin + 1; ptr != end; ++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.
это лишняя строка
|
|
||
| if(*ptr == *(ptr - 1)) ++currentLength; | ||
| 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(currentLength > maxLength){ | ||
| maxLength = currentLength; | ||
| best_begin = current_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.
а это снова я - лишняя строка
| PrintArray(begin + Limiter, end, Limiter, false); | ||
| } else { | ||
| 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.
Достаточно много дублирования
|
|
||
| if (firstCall) std::cout << "["; | ||
|
|
||
| size_t remaining = 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.
Можно использовать limiter и не плодить переменные, тем более используемую единожны
Часто используют понятную конструкцию limiter = std::min(limiter, end - begin)
|
|
||
| void SwapPtr(/* write arguments here */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| // В тестах есть три варианта каким могут быть аргументы 1-неконстартные указатели, 2 - константные указатели, 3 - указатели на указатели |
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 SwapPtr(/* write arguments here */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| // В тестах есть три варианта каким могут быть аргументы 1-неконстартные указатели, 2 - константные указатели, 3 - указатели на указатели | ||
| // Тогда воспользуемся перегрузкой функций как из лекций. У нас тогда будет три функции одинаковые по содержанию и разные |
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.
Посколько сам swap не должен менять значения то достаточно константной версии
| void SwapPtr(int**& a, int**& b) { | ||
| int** tmp = a; | ||
| a = b; | ||
| b = tmp; |
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.
Можно было обойтись одной версией
| case 'A': return 5; | ||
| default: return 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.
Это излишне, так как буквы имеет тоже свой порядок, в данном случае при увеличении оценок придется переписывать функцию, не очень хорошая идея
| rhs.birth_date.year, | ||
| rhs.birth_date.month, | ||
| rhs.birth_date.day); | ||
| } |
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 который использует ссылки
- Лучше написать отдельный оператор для даты и в данном случае использовать не каждое поле а сразу сравнение класса даты, поскольку там нет особенностей в поведении
- Действия с оценкой и курсом излишни, поскольку можно просто в левый операнд отправиль rhs.field, а в правый lhs.field в нужных местах
| } | ||
| os << "DEST"; | ||
| } | ||
|
|
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 readIndex = 0; readIndex < array.size(); ++readIndex) { | ||
| if (predicate(array[readIndex])) { | ||
| if (writeIndex != readIndex) { | ||
| array[writeIndex] = std::move(array[readIndex]); |
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::move не очень корректно, так как работа с фундаментальным типом, скопировать будет лучше
| //Итераторы ведут себя примерно также как указатели, но они могут быть и не указателями, а хоть чем. Это абстракция некая над указателем, если можно так сказать | ||
| */ | ||
|
|
||
| using IteratorsPairReturn = std::pair<std::vector<int>::const_iterator,std::vector<int>::const_iterator> ; |
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.
- лишний пробел в конце, и можно было ограничится псевдонимом для итератора, так как он дублируется
- не хватает пробела после запятой
| } | ||
| else if (*iter >= *maxIt) { | ||
| maxIt = iter; | ||
| } |
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.
проблемы с выравниванием кода
| os << "+"; | ||
| } else { | ||
| os << "-"; | ||
| } |
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 (int i = from; i > to; i += step) { | ||
| rangedArray.push_back(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.
дублирование кода
| } | ||
| else if (step < 0 && from > to) { | ||
| countSteps = (from - to - 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.
можно переписать с тернарным оператором при оъявлении
| outputArray.push_back(inputArray[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.
не уверен в выгодности двух проходов
|
|
||
| size_t uniqueCount = 1; | ||
| for (size_t i = 1; i < size; ++i) { | ||
| if (inputArray[i] != inputArray[i-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.
не хватает пробелов вокруг оператора -
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.
@Dsv9toy Данил, проверил первые 3 недели
- неплохой код, но местами много дублирования
- много ошибок по стилю кода, небрежность в выравнивании
No description provided.