Skip to content
This repository was archived by the owner on Sep 27, 2022. It is now read-only.

FileMap with JUnit (#Задание4)#598

Open
AndrewTimokhin wants to merge 19 commits intodkomanov:masterfrom
AndrewTimokhin:master
Open

FileMap with JUnit (#Задание4)#598
AndrewTimokhin wants to merge 19 commits intodkomanov:masterfrom
AndrewTimokhin:master

Conversation

@AndrewTimokhin
Copy link
Contributor

В данном задании имплементированы те интерфейсы, которые запрашивались ранее. Т.е. это второй пункт решения этого задания (второй отослан ранее). Помимо методов, которые перечислены ранее, добавлен метод чтения данных с жесткого диска и также записи на жесткий диск. Тесты были написаны в среде разработки NetBeans 8.0.1. И успешно прошли. Класс JUnit служит для проверки логики работы с фабрикой, то что она производит и т.д. и т.п. Можно скомпилировать исходные файлы в jar, тем самым собрав библиотеку и успешно ею пользоваться. Все замечания будут учтены!

@AndrewTimokhin
Copy link
Contributor Author

здесь не перегружай toString(), а передавай строку в конструктор базового класса
Comment: изменено super(description);

логика чтения базы с диска. в какой функции это реализовано?
Comment: Класс Reader;

где тесты?
Comment: В папке test;

однобуквенные имена переменных убрать
Comment: Убрано, за исключением счетчиков цикла, а также сокращений типа temporaryFile => tmpFile. Руководствовался этим при написании программы http://www.ibm.com/developerworks/java/library/ws-tip-namingconv.html

комментарии убрать
Comment: убраны.

завести свои типы для исключений и бросать их
Comment: Можно обойтись и уже имеющимися типами исключений. Однако, в качестве продвинутого исключения применено сцепленное исключение KeyNullAndNotFound, которое имеет под собой причину и показывает механизм сцепки исключений в JAVA.

идиома копирования массива. найти в стандартной библиотеке.
Comment: System.arraycopy(collection,0,temp,0,collection.length);
System.arraycopy(collection, 0, temp, 0, i);
if (i < collection.length) {
System.arraycopy(collection, i + 1, temp, i,
collection.length - i - 1);
}

логики с 16 папками и 16 файлами
Comment: Логика была прооптимизирована цель не создания тучи массивов из хэшмэпов. Мы просто делаем один хэшмэп и распределяем его по его хэшкоду ключа в директорию, по хэшкоду значения в файл, таким образом мы получаем полный адрес, куда записывать соответствующую пару key/value. Такой подход позволят нам дефрагментировать память и несколько увеличивает производительность.

flag, time - дать осмысленные названия
Comment: переделано.

вывода на System.out.println
Comment: убрано.

никаких паблик-полей
Comment: обернул все в элементарные геттеры и сеттеры

зачем в TableImplement.java везде проверка map != null, в какой момент это может вдруг стать верным?<<
Comment: с целью оптимизации- отсутствие ключей и значений, при не вызове метода удаления таблицы по контракту не значит, что таблицы нет. Это значит, что она тупо пуста. Если у нас есть папка в винде пустая, винда же ее не удалит, если только мы не запросим, аналогично тут.

new ArrayList() - ставить типы дженерика в угловых скобках, убрать касты
Comment:
List list = new ArrayList(time);
return list;

catch (IOException e) {
// do nothing
}
Comment: сделан вменяемый вывод.

rollback() всегда должен возвращать ноль?
Comment: переделано.

в тестах после всех действий проверять всё состояние. после коммита - сделать геты по всем релевантным ключам, например
Comment: тесты были добавлены еще.
Результат тестирования:
1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Точка в текстах исключений не ставится: см. стандартную библиотеку за примерами.
meth - необычное сокращение

Сообщение об ошибке не описывает причину проблемы и не помогает отладить приложение. Написать нормальное.

@vpavlenko
Copy link
Collaborator

Приложить к пул-реквесту скриншот с code coverage.

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
Collaborator

Choose a reason for hiding this comment

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

Зависимость на базу, убрать

Copy link
Contributor Author

Choose a reason for hiding this comment

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

убраны все зависимости от базы

@vpavlenko
Copy link
Collaborator

Я всё ещё не увидел скриншота с code coverage. Поставь себе IDE которая может породить code coverage (хоть trial-версию Intellij), породи чиселки и приложи скриншот.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ровно этот же код есть в TableProviderImplements, и повтор кода до сих пор не убран.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Разумеется я бы сделал в JAVA8 так: мы знаем, что появилось ключевое слово default, туда в интерйес загоняем метод и имплементим его в классе. Т.к. наследование по интерфейсам у нас безграничное по горизонтали (в отличии от классов)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

но с jdk8 проблемы на машине

Copy link
Collaborator

Choose a reason for hiding this comment

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

google "cloud ide free"

Conflicts:
	src/ru/fizteh/fivt/students/AndrewTimokhin/FileMap/JUnit/Main/Main.java
@AndrewTimokhin
Copy link
Contributor Author

Все исправлено.

@AndrewTimokhin
Copy link
Contributor Author

Все тесты прошли 100%. Прикрепляю скрин с коверадж код.
default

@AndrewTimokhin
Copy link
Contributor Author

For tests used JaCoCoverage analysis by EclEmma integrated in NetBeans and support JDK8.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Этот код должен быть в интерпретаторе

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants