Conversation
* add 1: add addition task * Add addition.cpp only * add 1: add char_changer task * add : add check_flags task
|
|
||
| /* 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.
немного некорректное описание проблемы
| std::cout << ", ...\n "; | ||
| count = 0; | ||
| first = true;}} | ||
| } |
There was a problem hiding this comment.
в данном случае очень большое дублирование кода, лучше было по месту использовать тернарный оператор, либо переписать функции, либо вынести в ещё одну функцию, оставив один 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.
поскольку мы не меняем самого значения, достаточно константной версии
| 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.
в данной задаче можно обойтись одной функцией, написан лишний код
| double sd = 0.0; | ||
| }; | ||
| double avg; | ||
| double sd;}; |
There was a problem hiding this comment.
вот как раз оставлять неинициализированные поля не принято
| 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.
два прохода. Можно реализовать эффективнее и считать оба параметра на одном походе
| 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.
конструкторы для структур излишни в данном случае, поскольку они тривиальные, компилятор бы справился и принято использовать данную возможность
| 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.
очень не приятное глазу оформление } на новой строке
| unsigned course; | ||
| Date birth_date; | ||
| }; No newline at end of file | ||
| Date birth_date;}; |
| 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.
это не плохо но можно выполнить с помощью 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.
должна быть пустая строка перед, кашу из непрерывного кода неудобно читать, иногда по смыслу нужно отбивать части кода одной пустой строкой, читающие программисты скажут спасибо
| /* 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.
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.
не уверен, что нужно давать возможность изменять по итераторам вне функции минимальный и максимальный элемент, а не только читать их, если этого не оговорено в ТЗ отдельно
| int x = 0; | ||
| int y = 0; | ||
| Coord2D() = default; | ||
| Coord2D(int x_, int y_) : x(x_), y(y_) {}}; |
There was a problem hiding this comment.
лишние конструкторы
| if (c.isEmpty()) { | ||
| os << "circle[]";} | ||
| else { | ||
| os << "circle[" << c.coord << ", r = " << c.radius << "]";} |
There was a problem hiding this comment.
в данном случае подходит тернарный оператор
| 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.
лишние конструкторы, компилятор сам справится с тривиальными конструкторами, для структур, их не принято писать
| std::ostream& operator<<(std::ostream& os, const CircleRegionList& list) { | ||
| if (list.empty()) { | ||
| os << "{}"; | ||
| return os;} |
There was a problem hiding this comment.
нужно } напистаь на новой строке и даже оставить потом пустую строку, чтобы отделить особый слуай от основной программы, также можно сразу писать return os << "{}";
| } else { | ||
| for (int i = from; i > to; i += step) { | ||
| ++count;} | ||
| } |
There was a problem hiding this comment.
Это лишний проход, можно предпосчитать правильное посчитать элементов по формуле, без прохода
| } else { | ||
| for (int i = from; i > to; i += step) { | ||
| result.push_back(i);} | ||
| } |
There was a problem hiding this comment.
можно по-другому переписать условия использовать тернарный оператор, но в данном случае дублирование кода, который можно избежать
18thday
left a comment
There was a problem hiding this comment.
@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.
было требование к вынесению определений вне класса, так неодобно ситать код, когда } не заканчивается на новой строке
| start = other.start; | ||
| end = other.end; | ||
| count = other.count;} | ||
| return *this;} |
There was a problem hiding this comment.
конструкторы и операторы присваивания пишутся с другими конструкторами
| public: | ||
| Queue() = default; | ||
| Queue(const std::stack<int>& s) { | ||
| std::stack<int> temp = s; |
There was a problem hiding this comment.
можно тогда сразу принять по значению
| Transfer(); | ||
| return outStack_.front(); } | ||
| const int& Back() const { | ||
| return const_cast<Queue*>(this)->Back();} |
There was a problem hiding this comment.
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.
неоптимальное сравнение, копирование излишне, причем мы копируем даже когда размеры разные
18thday
left a comment
There was a problem hiding this comment.
@karskanovas следующие моменты:
- плохо читаемый стиль кода
- UB const_cast
- дублирование кода
- неоптимальный оператор сравнения Queue с безусловным копированием двух очереденей
No description provided.