Skip to content
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

Hw04 lru cache #1

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Hw04 lru cache #1

wants to merge 48 commits into from

Conversation

dmitryt
Copy link
Owner

@dmitryt dmitryt commented Apr 13, 2020

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

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

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

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

Antonboom
Antonboom previously approved these changes Apr 15, 2020
Copy link
Collaborator

@Antonboom Antonboom left a comment

Choose a reason for hiding this comment

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

https://otus.ru/teacher-lk/homework/52091/6597/

Здравствуйте!
Хорошая работа. Не хватает одного тестика и есть вопросы по вашем комментариям, а так все ок. Рекомендовал бы внимательнее читать ТЗ к задачам.

Принято
9 / 10


middle := l.Back().Next // 20
middle := l.Back().Prev // 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Видно, что вы плохо читали ТЗ.
Там приведена структура ожидаемого списка

Copy link
Owner Author

Choose a reason for hiding this comment

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

Но погодите... Back() вернет последний элемент, l.Back().Next вернет nil, но никак не средний. Или я что-то не так понял?

Copy link
Owner Author

Choose a reason for hiding this comment

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

все, увидел... Мой фолт :(

}

// Just for debugging purpose
func (l *list) display() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Рекомендовал бы в display отдавать io.Writer

Copy link
Owner Author

Choose a reason for hiding this comment

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

попробую, спасибо

Remove(i *listItem)
MoveToFront(i *listItem)

display()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Приватные методы в интерфейсе - редкий кейс

Copy link
Owner Author

Choose a reason for hiding this comment

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

спасибо, учту

require.Equal(t, []interface{}{nil, false}, wrap(c.Get("ccc")))
})

t.Run("check cache capacity", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не хватает теста, где вы проверяете не просто размер очередь и её очистку через Clear(), но и проверяете перестановку элементов в очереди:

  • добавили элементы
  • дернули элементы несколько раз
  • добавили новый, проверили, что из очереди удалился наименее используемый

}

func (c *lruCache) Clear() {
for key, item := range c.items {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно пересоздать items и queue аналогично конструктору. GC выполнит свою работу

return found
}

func (c *lruCache) Clear() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mux потерялся?

Copy link
Owner Author

Choose a reason for hiding this comment

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

упс

c.queue.Remove(item)
}
c.items[key] = c.queue.PushFront(cacheItem{key: key, value: value})
if c.queue.Len() > c.capacity {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно вынести в отдельный приватный purge метод

c.mux.Lock()
defer c.mux.Unlock()
item, found := c.items[key]
// it's strange, that I cannot return c.items[key] directly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Что странного?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ну допустим в этом кейсе неактуально, но было бы удобно делать return c.items[key], вместо создания дополнительных переменных, но почему-то компилятор не дает такого делать

defer c.mux.Unlock()
item, found := c.items[key]
if found {
// Looks like I cannot reassign value in a such way
Copy link
Collaborator

Choose a reason for hiding this comment

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

Почему так выглядит?
Можно зарессайнить значение + MoveToFront?

… hw04_lru_cache

# Conflicts:
#	.travis.yml
#	README.md
#	hw04_lru_cache/cache.go
#	hw04_lru_cache/cache_test.go
#	hw04_lru_cache/go.mod
#	hw04_lru_cache/go.sum
#	hw04_lru_cache/list.go
#	hw04_lru_cache/list_test.go
#	hw05_parallel_execution/README.md
#	hw05_parallel_execution/go.mod
#	hw05_parallel_execution/go.sum
#	hw05_parallel_execution/run_test.go
#	hw06_pipeline_execution/pipeline_test.go
#	hw07_file_copying/test.sh
#	hw08_envdir_tool/README.md
#	hw08_envdir_tool/test.sh
#	hw09_generator_of_validators/test.sh
#	hw11_telnet_client/test.sh
@dmitryt
Copy link
Owner Author

dmitryt commented May 1, 2020

никаких изменений, случайно запушил несколько коммитов, подмерджил мастер

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