-
Notifications
You must be signed in to change notification settings - Fork 0
Soft cache #2
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: master
Are you sure you want to change the base?
Soft cache #2
Conversation
SoftCache/src/SoftCacheMap.java
Outdated
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
|
|
||
| public class SoftCacheMap implements Cache<Integer, String> { |
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.
А почему не сделать универсальную реализацию?
SoftCache/src/SoftCacheMap.java
Outdated
| public class SoftCacheMap implements Cache<Integer, String> { | ||
|
|
||
| private HashMap<Integer, SoftReference<String>> hashMap1; | ||
| private HashMap<SoftReference<String>, HashSet<Integer>> hashMap2; |
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.
Не очень-то говорящие названия
SoftCache/src/SoftCacheMap.java
Outdated
| } else { | ||
| hashMap2.put(hashMap1.get(key), new HashSet(key)); | ||
| } | ||
| while (delete()) { |
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.
Почему не сделать метод cleanup (например), который будет выполнять всю чистку. delete - достаточно размытое название, а пустой цикл смотрится совсем странно. Ну, и вызывать метод много раз вместо того, чтоб пробежаться в одном цикле по всем элементам referenceQueue - не слишком-то практично
SoftCache/src/SoftCacheMap.java
Outdated
| if (hashMap2.get(hashMap1.get(key)) != null) { | ||
| hashMap2.get(hashMap1.get(key)).add(key); | ||
| } else { | ||
| hashMap2.put(hashMap1.get(key), new HashSet(key)); |
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.
Зачем столько звать hashMap1.get(key)? Ты же сам только что туда положил этот элемент.
SoftCache/.idea/misc.xml
Outdated
| @@ -0,0 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
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.
Все эти *.xml и .iml файлы тоже не очень-то нужны в репозитории. Они специфичны конкретно для твоей машины, остальным про них знать ничего не надо
SoftCache/src/SoftCacheMap.java
Outdated
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
|
|
||
| public class SoftCacheMap implements Cache<Integer, String> { |
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.
Вообще говоря, в задании ещё была речь про небольшой тестик, демонстрирующий работу. А ещё про то, что хорошо бы препятствовать вытеснению объектов, к которым недавно обращались. То есть можно было бы держать небольшую очередь с Strong Reference на объекты, которые только что запрашивали.
SoftCache/src/SoftCacheMap.java
Outdated
| SoftReference reference = (SoftReference) referenceQueue.poll(); | ||
| if (reference != null) { | ||
| if (hashMap1.entrySet().contains(reference)) { | ||
| HashSet localSet = hashMap2.remove(reference); |
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.
Дженериковые классы создавай с их параметрами: HashSet<Integer>. Тогда бы цикл был не по непонятным Object
К конструктору SoftCacheMap это тоже относится. Idea же даже посвечивать должна такое
SoftCache/src/SoftCacheMap.java
Outdated
| if (hashMap1.get(key).get() != null) { | ||
| return hashMap1.get(key).get(); | ||
| } | ||
| return null; |
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 hashMap1.get(key).get(); написать. Хотя на наличие ключа в мапе тоже стоит проверять
SoftCache/src/SoftCacheMap.java
Outdated
| @Override | ||
| public void put(Integer key, String value) { | ||
| hashMap1.put(key, new SoftReference(value, referenceQueue)); | ||
| if (hashMap2.get(hashMap1.get(key)) != null) { |
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.
Если что, есть метод containsKey
SoftCache/src/SoftCacheMap.java
Outdated
|
|
||
| @Override | ||
| public String remove(Integer key) { | ||
| if (hashMap1.get(key).get() != null) { |
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.
Всегда проверяй на наличие ключа, прежде чем использовать полученное значение. NullPointerException в Java самая распространенная ошибка
DanAnastasyev
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.
Напиши хоть какой-нибудь код, демонстрирующий, что это работает
SoftCache/src/SoftCacheMap.java
Outdated
| import java.util.HashSet; | ||
|
|
||
| public class SoftCacheMap implements Cache<Integer, String> { | ||
| public class SoftCacheMap implements Cache<Integer, Object> { |
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.
Не. У тебя же шаблонный интерфейс. Почему бы не сделать шаблонную же реализацию?
SoftCache/src/SoftCacheMap.java
Outdated
| while (!done) { | ||
| SoftReference reference = (SoftReference) referenceQueue.poll(); | ||
| if (reference != null) { | ||
| if (cache.entrySet().contains(reference)) { |
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.
А что возвращает entrySet(), кстати?
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.
Set из пар <key, value>. Тут, видимо, нужен values() - он возвращает коллекцию из значений
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 (subsidiaryMap.containsKey(reference))
SoftCache/src/SoftCacheMap.java
Outdated
| if (subsidiaryMap.containsKey(reference)) { | ||
| subsidiaryMap.get(reference).add(key); | ||
| } else { | ||
| subsidiaryMap.put(reference, new HashSet<>(key)); |
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.
new HashSet<>(key) - что, по-твоему, тут происходит?
SoftCache/src/SoftCacheMap.java
Outdated
| import java.util.HashSet; | ||
| import java.util.LinkedList; | ||
|
|
||
| public class SoftCacheMap<V> implements Cache<Integer, V> { |
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.
Что же всё-таки мешает тебе сделать его SoftCacheMap<K, V>? Зачем тебе там Integer?
SoftCache/src/SoftCacheMap.java
Outdated
|
|
||
| private HashMap<Integer, SoftReference<V>> cache; | ||
| private HashMap<SoftReference<V>, HashSet<Integer>> subsidiaryMap; | ||
| private ReferenceQueue<? super Object> referenceQueue; |
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.
А что означает <? super Object>, и зачем тебе это тут?
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.
Ограничение, означает что мы при обращении к элементам очереди получим Object, а записать в очередь можем любого наследника Object.
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.
А с учетом того, что все классы - наследники Object, не проще ли написать ReferenceQueue<Object>?
Но с учетом того, что в ней могут быть только объекты типа V, напиши лучше ReferenceQueue<V>
И почитай на всякий случай вот тут про wildcards в шаблонах
SoftCache/src/SoftCacheMap.java
Outdated
| cache1.put(0, new Integer(0)); | ||
|
|
||
| /** | ||
| * должен быть 0 |
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.
Вообще говоря, в задании говорилось про написания юнит-теста. Тут же так и просится assert вместо фразы "должен быть 0".
Перенеси это всё в класс с junit-тестом. Можешь почитать где-нибудь тут или посмотреть в основном репозитории, как они делаются
No description provided.