Skip to content

Conversation

@nrsxx
Copy link
Owner

@nrsxx nrsxx commented Apr 23, 2017

No description provided.

import java.util.HashMap;
import java.util.HashSet;

public class SoftCacheMap implements Cache<Integer, String> {

Choose a reason for hiding this comment

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

А почему не сделать универсальную реализацию?

public class SoftCacheMap implements Cache<Integer, String> {

private HashMap<Integer, SoftReference<String>> hashMap1;
private HashMap<SoftReference<String>, HashSet<Integer>> hashMap2;

Choose a reason for hiding this comment

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

Не очень-то говорящие названия

} else {
hashMap2.put(hashMap1.get(key), new HashSet(key));
}
while (delete()) {

Choose a reason for hiding this comment

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

Почему не сделать метод cleanup (например), который будет выполнять всю чистку. delete - достаточно размытое название, а пустой цикл смотрится совсем странно. Ну, и вызывать метод много раз вместо того, чтоб пробежаться в одном цикле по всем элементам referenceQueue - не слишком-то практично

if (hashMap2.get(hashMap1.get(key)) != null) {
hashMap2.get(hashMap1.get(key)).add(key);
} else {
hashMap2.put(hashMap1.get(key), new HashSet(key));

Choose a reason for hiding this comment

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

Зачем столько звать hashMap1.get(key)? Ты же сам только что туда положил этот элемент.

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>

Choose a reason for hiding this comment

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

Все эти *.xml и .iml файлы тоже не очень-то нужны в репозитории. Они специфичны конкретно для твоей машины, остальным про них знать ничего не надо

import java.util.HashMap;
import java.util.HashSet;

public class SoftCacheMap implements Cache<Integer, String> {

Choose a reason for hiding this comment

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

Вообще говоря, в задании ещё была речь про небольшой тестик, демонстрирующий работу. А ещё про то, что хорошо бы препятствовать вытеснению объектов, к которым недавно обращались. То есть можно было бы держать небольшую очередь с Strong Reference на объекты, которые только что запрашивали.

SoftReference reference = (SoftReference) referenceQueue.poll();
if (reference != null) {
if (hashMap1.entrySet().contains(reference)) {
HashSet localSet = hashMap2.remove(reference);

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 же даже посвечивать должна такое

if (hashMap1.get(key).get() != null) {
return hashMap1.get(key).get();
}
return null;

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(); написать. Хотя на наличие ключа в мапе тоже стоит проверять

@Override
public void put(Integer key, String value) {
hashMap1.put(key, new SoftReference(value, referenceQueue));
if (hashMap2.get(hashMap1.get(key)) != null) {

Choose a reason for hiding this comment

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

Если что, есть метод containsKey


@Override
public String remove(Integer key) {
if (hashMap1.get(key).get() != null) {

Choose a reason for hiding this comment

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

Всегда проверяй на наличие ключа, прежде чем использовать полученное значение. NullPointerException в Java самая распространенная ошибка

Copy link

@DanAnastasyev DanAnastasyev left a comment

Choose a reason for hiding this comment

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

Напиши хоть какой-нибудь код, демонстрирующий, что это работает

import java.util.HashSet;

public class SoftCacheMap implements Cache<Integer, String> {
public class SoftCacheMap implements Cache<Integer, Object> {

Choose a reason for hiding this comment

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

Не. У тебя же шаблонный интерфейс. Почему бы не сделать шаблонную же реализацию?

while (!done) {
SoftReference reference = (SoftReference) referenceQueue.poll();
if (reference != null) {
if (cache.entrySet().contains(reference)) {

Choose a reason for hiding this comment

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

А что возвращает entrySet(), кстати?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Set из пар <key, value>. Тут, видимо, нужен values() - он возвращает коллекцию из значений

Copy link
Owner Author

Choose a reason for hiding this comment

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

А еще лучше сделать if (subsidiaryMap.containsKey(reference))

if (subsidiaryMap.containsKey(reference)) {
subsidiaryMap.get(reference).add(key);
} else {
subsidiaryMap.put(reference, new HashSet<>(key));

Choose a reason for hiding this comment

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

new HashSet<>(key) - что, по-твоему, тут происходит?

import java.util.HashSet;
import java.util.LinkedList;

public class SoftCacheMap<V> implements Cache<Integer, V> {

Choose a reason for hiding this comment

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

Что же всё-таки мешает тебе сделать его SoftCacheMap<K, V>? Зачем тебе там Integer?


private HashMap<Integer, SoftReference<V>> cache;
private HashMap<SoftReference<V>, HashSet<Integer>> subsidiaryMap;
private ReferenceQueue<? super Object> referenceQueue;

Choose a reason for hiding this comment

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

А что означает <? super Object>, и зачем тебе это тут?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ограничение, означает что мы при обращении к элементам очереди получим Object, а записать в очередь можем любого наследника Object.

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 в шаблонах

cache1.put(0, new Integer(0));

/**
* должен быть 0

Choose a reason for hiding this comment

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

Вообще говоря, в задании говорилось про написания юнит-теста. Тут же так и просится assert вместо фразы "должен быть 0".
Перенеси это всё в класс с junit-тестом. Можешь почитать где-нибудь тут или посмотреть в основном репозитории, как они делаются

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.

3 participants