-
Notifications
You must be signed in to change notification settings - Fork 33
Карсканова Светлана #29
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
* add 1: add addition task * Add addition.cpp only * add 1: add char_changer task * add : add check_flags task
| << static_cast<int>(bytes[i]); } | ||
| } | ||
| std::cout << std::dec << 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 от типа, правильнее было вызвать третью функцию которая уже печатает требуемое представление
| } else { | ||
| for (size_t i = 0; i < sizeof(double); ++i) { | ||
| std::cout << std::hex << std::setw(2) << std::setfill('0') | ||
| << static_cast<int>(bytes[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.
в данном случае тоже дублирование, можно было использовать тернарный оператор в нужных местах, либо вынести настройку std::cout
| } | ||
| current_start = ptr; | ||
| current_len = 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.
вот так корректно и так должно быть везде
| /* return_type */ FindLongestSubsequence(/* ptr_type */ begin, /* ptr_type */ end, /* type */ count) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| const char* FindLongestSubsequence(const char* begin, const char* end, size_t& count) { //из за ошибки про то, что ожидается изменяемый массив | ||
| return FindLongestSubsequence(const_cast<char*>(begin), const_cast<char*>(end), 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.
Это некорректно, это UB, должно быть наоборот, снимать const можно, если изначально был объект не конст, то есть из версии без константности, можно вызвать константную (где весь код из текущей неконстантной версии), а затем снять константность, но никак не наоборот, в данном случае это UB
|
|
||
| /* return_type */ FindLongestSubsequence(/* ptr_type */ begin, /* ptr_type */ end, /* type */ count) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| const char* FindLongestSubsequence(const char* begin, const 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.
немного некорректное описание проблемы
| std::cout << ", ...\n "; | ||
| count = 0; | ||
| first = 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.
в данном случае очень большое дублирование кода, лучше было по месту использовать тернарный оператор, либо переписать функции, либо вынести в ещё одну функцию, оставив один for
| int* temp = a; | ||
| a = b; | ||
| b = temp;} | ||
| void SwapPtr(const int*& a, const int*& b) {//const 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.
поскольку мы не меняем самого значения, достаточно константной версии
| void SwapPtr(int**& a, int**& b) { //int | ||
| int** temp = a; | ||
| a = b; | ||
| b = temp;} No newline at end of file |
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 sd = 0.0; | ||
| }; | ||
| double avg; | ||
| double sd;}; |
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 mean = sum / data.size(); | ||
| double variance_sum = 0.0; | ||
| for (int x : data) { | ||
| variance_sum += (x - mean) * (x - mean);} |
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.
два прохода. Можно реализовать эффективнее и считать оба параметра на одном походе
| unsigned day = 0; | ||
| Date() = default; | ||
| Date(unsigned y, unsigned m, unsigned d) | ||
| : year(y), month(m), day(d) {}}; |
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 operator>(const Date& lhs, const Date& rhs) { | ||
| return rhs < lhs;} | ||
| bool operator>=(const Date& lhs, const Date& rhs) { | ||
| return !(lhs < rhs);} |
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.
очень не приятное глазу оформление } на новой строке
| unsigned course; | ||
| Date birth_date; | ||
| }; No newline at end of file | ||
| Date 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.
эх
| return lhs.course > rhs.course; | ||
| if (lhs.birth_date != rhs.birth_date) | ||
| return lhs.birth_date < rhs.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() < std::tie() в левом операнде можно располагать не только поля lhs но и rhs, тем самым добиваясь, что они сравниваются на <, а другие на >
| static_cast<uint8_t>(rhs); | ||
| result &= static_cast<uint8_t>(CheckFlags::ALL); | ||
| return static_cast<CheckFlags>(result);} | ||
| bool operator&(CheckFlags lhs, CheckFlags rhs) { |
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 */ Filter(/* args */) { | ||
| throw std::runtime_error{"Not implemented"}; | ||
| } No newline at end of file | ||
| void Filter(std::vector<int>& v, bool (*pred)(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.
v не подходит в качестве названия ) data, values будет лучше
| if (*it >= *max_it) { | ||
| max_it = it;} | ||
| } | ||
| return std::make_pair(min_it, max_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.
не уверен, что нужно давать возможность изменять по итераторам вне функции минимальный и максимальный элемент, а не только читать их, если этого не оговорено в ТЗ отдельно
| int x = 0; | ||
| int y = 0; | ||
| Coord2D() = default; | ||
| Coord2D(int x_, int y_) : x(x_), y(y_) {}}; |
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 (c.isEmpty()) { | ||
| os << "circle[]";} | ||
| else { | ||
| os << "circle[" << c.coord << ", r = " << c.radius << "]";} |
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.
в данном случае подходит тернарный оператор
| using CircleRegion = std::pair<Circle, bool>; | ||
| unsigned int radius = 1; | ||
| Circle() = default; | ||
| Circle(const Coord2D& c, unsigned int r = 1) : coord(c), radius(r) {} |
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::ostream& operator<<(std::ostream& os, const CircleRegionList& list) { | ||
| if (list.empty()) { | ||
| os << "{}"; | ||
| return 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.
нужно } напистаь на новой строке и даже оставить потом пустую строку, чтобы отделить особый слуай от основной программы, также можно сразу писать return os << "{}";
| } else { | ||
| for (int i = from; i > to; i += step) { | ||
| ++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.
Это лишний проход, можно предпосчитать правильное посчитать элементов по формуле, без прохода
| } else { | ||
| for (int i = from; i > to; i += step) { | ||
| result.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.
можно по-другому переписать условия использовать тернарный оператор, но в данном случае дублирование кода, который можно избежать
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.
@karskanovas основные моменты
- C-style каст не используют в C++ кода
- плохо читаемый стиль кода (
}- принято на новой строке, иногда не хватает пустых строк для удобного оттеделния логических частей функйии ) - UB const_cast
- дублирование кода
| bool operator==(const Stack& other) const { | ||
| return data == other.data;} | ||
| bool operator!=(const Stack& other) const { | ||
| return data != other.data;} |
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.
было требование к вынесению определений вне класса, так неодобно ситать код, когда } не заканчивается на новой строке
| start = other.start; | ||
| end = other.end; | ||
| count = other.count;} | ||
| return *this;} |
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.
конструкторы и операторы присваивания пишутся с другими конструкторами
| public: | ||
| Queue() = default; | ||
| Queue(const std::stack<int>& s) { | ||
| std::stack<int> temp = s; |
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.
можно тогда сразу принять по значению
| Transfer(); | ||
| return outStack_.front(); } | ||
| const int& Back() const { | ||
| return const_cast<Queue*>(this)->Back();} |
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 const_cast так как можетприменяться для константного объекта
| std::swap(outStack_, other.outStack_);} | ||
| bool operator==(const Queue& other) const { | ||
| Queue copy1 = *this; | ||
| Queue copy2 = other; |
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.
неоптимальное сравнение, копирование излишне, причем мы копируем даже когда размеры разные
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.
@karskanovas следующие моменты:
- плохо читаемый стиль кода
- UB const_cast
- дублирование кода
- неоптимальный оператор сравнения Queue с безусловным копированием двух очереденей
No description provided.