Skip to content

HW4 is completed#4

Open
ezhk wants to merge 11 commits intomasterfrom
hw04_lru_cache
Open

HW4 is completed#4
ezhk wants to merge 11 commits intomasterfrom
hw04_lru_cache

Conversation

@ezhk
Copy link
Copy Markdown
Owner

@ezhk ezhk commented Jun 21, 2020

Домашнее задание №4 «LRU-кэш»

Критерии оценки

  • Пайплайн зелёный - 4 балла
  • Добавлены новые юнит-тесты для списка - 1 балл
  • Добавлены новые юнит-тесты для кэша (включая тест на логику
    выталкивания из кэша редко используемых элементов) - до 3 баллов
  • Понятность и чистота кода - до 2 баллов

Зачёт от 7 баллов

Copy link
Copy Markdown
Collaborator

@kulti kulti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужно поправить работу списка, чтобы было О(1). После этого готов принять ДЗ на 8/10. Чтобы получить еще 2 балла, нужно поработать над чистотой и понятностью (в первую очередь замечания про Remove, Clear и ShowElements).

l.PushFront(i.Value)
}

func (l *list) ShowElements() []listItem {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Странный метод. Судя по всему, нужен только для тестов. Предлагаю не загрязнять API и перенести этот метод в виде вспомогательной функции в тесты.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрал этот метод в следующем коммите.
Согласен, тут скорее было бы уместнее сделать просто метод String().

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А зачем String? И в каком формате должен быть результат такого метода?

list не знает, что в нем хранится. Обычно это знает тот, кто в него кладет. Если ему зачем-то нужно показывать список в виде строки, то он сам это реализует, и вывод будет в том формате, в каком ему нужно.

type list struct {
// Place your code here
// using on delete validation
elementsSet map[*listItem]struct{}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Работа с map - это амортизированное О(1), т.е. О(1), но не всегда. В задании сказано, что все операции должны быть за О(1). Если хочется оставить усложнение (удалять только существующие элементы), то предлагаю подумать над другим подходом.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Усложнение проверок добавили, а тестов на этот случай нет. Если хотите оставить, то еще и тесты нужно будет добавить.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрал Set реализацию.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И я правильно понимаю, что коллизиями мы обычно пренебрегаем и всё же считаем
что словарь и Set — O(1)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я так никогда не считаю :) Если интересно, вот старая, но, вроде, еще актуальная статья, как map устроен в go - https://www.ardanlabs.com/blog/2013/12/macro-view-of-map-internals-in-go.html. Коллизии - это норма, просто он старается держать их в разумных пределах.

Если можно сделать лучше: без лишних аллокаций, циклов etc, то лучше без них и сделать.

for c.Queue.Len() > 0 {
c.Queue.Remove(c.Queue.Back())
}
c.Items = make(map[Key]*listItem)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему для map тут создается новый экзепляр, а для списка удаляются элементы? Чем обусловлен разный подход к чистке?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, неоднозначно получилось, поправил во втором коммите.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC корректно отловит существующий список ни к чему не привязанный?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, раз ссылок на объект не осталось, то GC его приберет.

@ezhk ezhk requested a review from kulti June 25, 2020 15:45
kulti
kulti previously approved these changes Jun 25, 2020
Copy link
Copy Markdown
Collaborator

@kulti kulti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спасибо за доработки. ДЗ принято - 10/10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants