g595.Manucharyan.task3 #279
Conversation
|
|
||
| import ru.mipt.java2016.homework.base.task2.KeyValueStorage; | ||
|
|
||
| import static java.lang.Math.max; |
There was a problem hiding this comment.
Здесь в последних двух абзацах довольно коротко поясняют за статик импорт, и когда его нужно/не нужно использовать.
Что касается конкретно данного случая, он не попадает ни под одно из описаний. Можешь, конечно, оставить, однако лично я удивлён, что тебе оказалось проще написать import static, нежели просто заюзать Math.max() в одном месте в коде.
There was a problem hiding this comment.
просто, когда я написал max() он предложил подключить это (там было несколько вариантов), я просто нажал enter и забил)
| public class ConcreteStrategyStringRandomAccess implements SerializationStrategyRandomAccess<String> { | ||
| @Override | ||
| public void serializeToFile(String value, RandomAccessFile output) throws IOException { | ||
| writeString(output, value); |
There was a problem hiding this comment.
Какая клёвая реализация... А точно убрать один уровень вызовов никак нельзя? Лично мне кажется, что я вижу, как можно это сделать.
И кстати, попробуй вернуть назад writeUTF, вроде должно побыстрее начать работать :)
There was a problem hiding this comment.
Ну, можно inline написать 😃
|
|
||
| private static final long MAX_CACHE_SIZE = 100L; | ||
|
|
||
| private SerializationStrategyRandomAccess<K> keySerializationStrategy; |
There was a problem hiding this comment.
final же можно сделать - вряд ли ты будешь менять стратегию в процессе работы над хранилищем.
| @Override | ||
| public boolean exists(K key) { | ||
| if (isClosed) { | ||
| return false; |
There was a problem hiding this comment.
Тут бы IllegalStateException был кстати. А вообще эту проверку лучше в отдельную функцию вынести. А вот в close() не надо, он при повторном закрытии ничего не должен делать.
There was a problem hiding this comment.
У меня в close() и не было ничего) А так да, мне приходила эта идея в голову, но я забил)
| if (isClosed) { | ||
| return false; | ||
| } | ||
| if (cache.containsKey(key)) { |
There was a problem hiding this comment.
Идея предложила тебе заменить на return cache.containsKey(key) || base.containsKey(key), а ты её игноришь. Не надо так.
There was a problem hiding this comment.
Ну так же понятней, нет? Типа тут односложные ифы, и сразу понятно, что происходит, иначе надо думать(
There was a problem hiding this comment.
Ну, не знаю, по мне так два однотипных условия в одном выглядят читабельнее. Вообще не суть.
| File file2 = new File(path, mapStorageName); | ||
| mapStorage = new RandomAccessFile(file2, "rw"); | ||
|
|
||
| if (file.exists() && file2.exists()) { |
There was a problem hiding this comment.
При создании RandomAccessFile в режиме rw создаётся и файл, который ты подал в качестве аргумента. Получается, ты только что создал файлы (а если не создал, то вылететл IOException), а потом ещё специально что-то проверяешь?
|
|
||
| if (file.exists() && file2.exists()) { | ||
| uploadDataFromStorage(); | ||
| } else if (!file.createNewFile() || file2.createNewFile()) { |
There was a problem hiding this comment.
createNewFile() вернёт false только когда файл уже существет, то есть exist() вернул true. Такая проверка есть двумя строчками выше, так что по-моему лучше здесь if убрать, да и вообще, наверное, весь if-else блок, оставить только вызов функции. Объясни мне, зачем это ветвление здесь вообще.
There was a problem hiding this comment.
true if the named file does not exist and was successfully created; false if the named file already exists. Если файл не удалось создать, то должно вернуться false, нет? Ветвление здесь вот зачем: есть два варианта, файлы уже есть, тогда загружаем их. Если же их нет, но и создать их не получилось, то что-то пошло не так
There was a problem hiding this comment.
Давай так.
exist возвращает true тогда и только тогда, когда файл есть.
createNewFile возвращает false тогда и только тогда, когда файл есть (ты привёл кусок документации).
[Тут чудеса матлогики]
В итоге получаем, что эти два условия эквивалентны.
| */ | ||
| public interface SerializationStrategyRandomAccess<Value> { | ||
|
|
||
| void serializeToFile(Value value, RandomAccessFile output) throws IOException; |
There was a problem hiding this comment.
Вместо RandomAccessFile лучше сделать DataOutput здесь - он является родителем RandomAccessFile, но при этом код становится гибче.
|
|
||
| void serializeToFile(Value value, RandomAccessFile output) throws IOException; | ||
|
|
||
| Value deserializeFromFile(RandomAccessFile input) throws IOException; |
There was a problem hiding this comment.
То же, но про DataInput. Как я и говорил на семинаре (и не на одном), лучше не цепляться за конкретную реализацию.
| private SerializationStrategyRandomAccess<V> valueSerializationStrategy; | ||
|
|
||
| //consist keys and offsets(the pair of begin and length) | ||
| private HashMap<K, Long> base = new HashMap<>(); |
There was a problem hiding this comment.
Лучше тип переменной сделать Map, ты же внутри класса не используешь его как HashMap.
There was a problem hiding this comment.
Странно, у меня так же было и во втором задании...
|
Я так понимаю, ты сжимаешь хранилище только в |
| private void reorganiseStorage() { | ||
| try { | ||
| File file1 = new File(pathname, storageName); | ||
| RandomAccessFile storage = new RandomAccessFile(file1, "rw"); |
There was a problem hiding this comment.
try-with-resources здесь хорошо подходит.
| RandomAccessFile storage = new RandomAccessFile(file1, "rw"); | ||
|
|
||
| File file = new File(pathname, "newStorage.txt"); | ||
| RandomAccessFile newStorage = new RandomAccessFile(file, "rw"); |
There was a problem hiding this comment.
Через DataOutputStream можно было бы наклепать, обернув BufferedOutputStream. Побыстрее должно работать.
There was a problem hiding this comment.
мне же нужно знать offset для каждого элемента, а у DataOutputStream нет такого функционала, или я что-то не понимаю?
There was a problem hiding this comment.
Да, стало работать быстрее, спасибо)
No description provided.