-
Notifications
You must be signed in to change notification settings - Fork 33
Еремин Даниил #36
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?
Еремин Даниил #36
Conversation
| void PrintMemory(double num, bool inverse = false) { | ||
| const size_t size = sizeof(double); | ||
| auto ptr = reinterpret_cast<const unsigned char*>(&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.
идет полное дублирование кода, после получения указателя и, зная sizeof типа num, можно было вызвать третью функцию без дублирования кода
| continue; | ||
| } | ||
|
|
||
| res = count < curLen ? const_cast<char*>(curBegin) : res; |
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::cout << *curent << (curent-1 == end ? "" : ", "); | ||
| } | ||
| ++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.
практически полное дублирование кода, можнобыло например по условию определить шаг и потом шагать с этим шагом, используя один for, и переписав немного логику
| @@ -1,6 +1,18 @@ | |||
| #include <stdexcept> | |||
|
|
|||
| void SwapPtr(int*& a, int*& b) { | |||
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** 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.
можно было написать одну функцию
| res.avg /= size; | ||
| res.sd /= size; | ||
| res.sd -= res.avg * res.avg; | ||
| res.sd = sqrt(res.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.
std::sqrt
| 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, чтобы изменить знак сравнения на противоположный можно поменять местами поля в левом и правом аргументе
| bool operator&(const CheckFlags& lhs, const CheckFlags& rhs) { | ||
| uint8_t all_uint8 = static_cast<uint8_t>(CheckFlags::ALL); | ||
| uint8_t lhs_uint8 = static_cast<uint8_t>(lhs) & all_uint8; | ||
| uint8_t rhs_uint8 = static_cast<uint8_t>(rhs) & all_uint8; |
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::ostream& operator<<(std::ostream& os, const CheckFlags& flags) { | ||
| //Хэш-таблица для быстрого преобразования uint8_t -> string | ||
| std::unordered_map<uint8_t, std::string> flagStr = { |
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<std::string> result; | ||
| //Резервирование места под элементы, чтобы не было лишних реаллокаций | ||
| result.reserve(16); |
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<size_t> FindAll(const std::vector<int>& data, bool(*predicat)(int)){ | ||
| std::vector<size_t> res; | ||
| if(predicat != 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.
лучше сделать короткую ветвь с выходом и убрать дополнительную вложенность для основного кода
|
|
||
| /* 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>& 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.
можно было использовать псевдоним для итераторов, чтобы был не такой длинный тип
|
|
||
| // Определение размера | ||
| size_t size = std::abs(to - from) % std::abs(step) == 0 ? | ||
| std::abs(to - from) / std::abs(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.
поехало форматирование
| for(int i = from; i > to; i += step){ | ||
| res.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.
дублирование кода
| if(in_.empty()) return out_.front(); | ||
| return in_.back(); | ||
| } | ||
| const int& Queue::Front() const { |
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.
кажется лучше упорядочить по одинаковым именам, нежели по схожим сигнатурам
| in_.push_back(value); | ||
| } | ||
| bool Queue::Pop(){ | ||
| if(in_.empty() && out_.empty()) 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.
есть метод Empty()
| bool Queue::Pop(){ | ||
| if(in_.empty() && out_.empty()) return false; | ||
| if(out_.empty() && !(in_.empty())){ | ||
| while(!(in_.empty())){ |
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 Queue::Pop(){ | ||
| if(in_.empty() && out_.empty()) return false; | ||
| if(out_.empty() && !(in_.empty())){ |
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.
вторая проверка уже лишняя, так как за счет условия выше уже известно что in_ не пуст
| return out_.back(); | ||
| } | ||
| const int& Queue::Back() const { | ||
| if(in_.empty()) return out_.front(); |
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 можно
| return in_.back(); | ||
| } | ||
| bool Queue::Empty() const { | ||
| if(in_.empty() && out_.empty()) 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.
получается можно сразу return in_.empty() && out_.empty();
| }; | ||
|
|
||
|
|
||
| Queue::Queue(std::stack<int>& stack) { |
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<int> tmp_rhs(rhs.in_.rbegin(), rhs.in_.rend()); | ||
| tmp_rhs.insert(tmp_rhs.end(), rhs.out_.begin(), rhs.out_.end()); | ||
|
|
||
| return tmp_lhs == tmp_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.
это дорого
- если размеры очередей разные, то получается все равно делаются копии, хотя уже известно, что очереди разные
- зная внутреннее устройство, можно обойтись без копий
| // Возвращает `std::vector<int>` - линейное представление буфера | ||
| std::vector<int> Vector() const; | ||
| // Доступ по индексу | ||
| int& operator[](size_t 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.
много лишних комментариев, тяжело читать код
| std::vector<int> data_; | ||
| }; | ||
|
|
||
| void Stack::Push(const int value) { |
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.
константность лишняя, если не принимается по ссылке
| // | ||
| RingBuffer(size_t size); | ||
| RingBuffer(size_t capacity, int value); | ||
| RingBuffer(const std::initializer_list<int> list); |
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.
const лишнее в данном случае
| std::vector<int>::iterator begin_ = data_.begin(); | ||
| }; | ||
|
|
||
| RingBuffer::RingBuffer(size_t size) : data_(size + 1), size_(0), end_(data_.begin()), begin_(data_.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.
а зачем размер увеличивать на единицу?
| } | ||
| if(size_ != 1){ | ||
| begin_ = begin_ + 1 == data_.end() ? data_.begin() + 1 : begin_ + 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.
часть кода можно не копироватьесли использовать TryPush
| } | ||
| if(size_ != 1){ | ||
| begin_ = begin_ + 1 == data_.end() ? data_.begin() + 1 : begin_ + 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.
Как-то сложно получилось из-за необходимости постояннодобавлять вычитать 1
| New.insert(New.begin(), 0); | ||
| New.resize(size + 1); | ||
| data_ = New; | ||
| } |
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.
@RIZONTE следующие моменты:
- местами много лишних комментариев декларирующих то что понятно из кода
- дублирование кода
- UB const_cast
No description provided.