#134 exception-refactoring

[vaa25]

connect to #134

[witjem]

• passed

[alex-anakin]

1. Пакет org.itevents.dao.mybatis.sql_session_dao.
2. sql в названии пакета говорит, что там классы с sql. Но в классах нет sql...
3. dao - повторяет название вышестоящего пакета. Непонятно, зачем.
4. этот пакет находится в пакете mybatis, но в классах этого пакета нет чего-то специфического для майбатиса. Разве что там происходят вызовы мапперов майбатиса. Но общее назначение классов этого пакета не связано с майбатисом. Вывод: необходимо переместить пакет и изменить его название исходя из назначения классов в нем.
5. Класс AbstractMyBatisDao.
6. в классе нет ничего, что связано с майбатисом. Вывод: необходимо изменить название класса.
7. Классы UserMyBatisDao, RoleMyBatisDao и остальные рядом с ними. Предлагаю подумать над тем, что именно эти классы делают, исходя из этого дать им название.
8. Когда изменена логика теста, то его название тоже нужно поменять. Пример: EventMyBatisDaoDbTest.expectNullWhenEventIsAbsent() - название ожидает null, а тест проходит, когда прилетает эксепшн.
9. Т.к. автор ПР улучшает код предыдущих коммитов, то обращу внимание, что указание конкретного исключения в тесте, например, public void shouldGetAllEvents() throws ParseException говорит о том, что тестовые методы писались руками без автовставки IDE. Один из подходов говорит, что нужно после каждого метода бросать Exception, это по дефолту использовано разработчиками IDEA. Второй подход - не бросать исключений вообще, это хорошо видно в примерах на сайте JUnit. Предлагаю все тесты делать в едином стиле @Test public void testSomething() throws Exception
10. Продолжаем бороться с логикой в контроллерах. Что мешает вместо этого кода в методе контроллера public void deactivateSubscription() { User user = userService.getAuthorizedUser(); userService.deactivateUserSubscription(user); } сделать так: public void deactivateSubscription() { userService.deactivateUserSubscription(); } а уже в сервисе сделать как-то так public void deactivateUserSubscription() { userDao.updateUser(getAuthorizedUser().setSubscribed(false)); } ?
11. Еще логика в контроллере: public void activateSubscription(@ModelAttribute FilterWrapper wrapper) { Filter filter = new FilterConverter().toFilter(wrapper); filter.setCreateDate(DateTimeUtil.getNowDate()); User user = userService.getAuthorizedUser(); filterService.addFilter(user, filter); userService.activateUserSubscription(user); } Можно же передать в сервис фильтрВраппер и там уже вертеть его как душе будет угодно. А контроллер выполнит только ту функцию, для которой он создан - принять запрос и передать вызов сервису.

[vaa25]

@alex-anakin названия если не нравятся то сразу говори на какие менять

[vaa25]

@alex-anakin

2. название говорит, что там SqlSessionDao, и там действительно дао основанное на SqlSession.
3. не повторяет, а уточняет, что там именно дао, а не исключения или мапперы
4. а наследование этих классов от батиса не есть что-то специфическое для батиса? общее назначение - работать с бд с помощью батиса, и только батиса
5. а наследование этого класса от батиса не есть что-то специфическое для батиса?
6. подумал и исходя из этого и дал. UserMyBatisDao - data access object working with DB and User via MyBatis. Или это не так?
7. автор ПР выполняет таск #134. а continuous refactoring никто не отменял. грузить не надо.
8. задел на админку. админ будет иметь право менять данные юзеров, а не только свои. но в целом согласен, думал об этом.
9. раньше так и было, но FilterConverter и FilterWrapper согласно указаниям Виктора перенесены в зону ответственности контроллера, так что сервисы о них ничего знать не должны.

[vaa25]

Виктор же сказал не закрывать пр.

[vaa25]

@alex-anakin

• 8 и 9 сделал
• 10 и 11 перенес в таск #148
• с остальным не согласен, аргументы выше

[alex-anakin]

1. В названии метода ошибка. Ловим одно, название другое. public ResponseEntity handleEventNotFoundControllerException(EntityNotFoundServiceException ex)
2. После мерджа мастера был удален EventTechnologySqlBuilder, а TechnologySqlBuilder, которого не было в мастере, как я понимаю, остался без рефакторинга. Выход: завести таску на рефакторинг одного билдера или порефакторить его в текущей ветке.

Думаю, это всё в данной ветке.

[vaa25]

1. Исправлю
2. EventTechnologySqlBuilder вошел в состав EventSqlBuilder, TechnologySqlBuilder да, нужно порефакторить. сделаю.
3. Я особо не присматривался к изменениям из мастера, там точно все в порядке?

ПС: огромное тебе спасибо, я думал это я у всех мух вылавливаю, оказывается есть еще куда стремиться)))

[alex-anakin]

1. There is magic number in class TechnologySqlBuilder sql.delete(sql.length() - 2, sql.length())
2. Tip from github - This branch has conflicts that must be resolved

[romach]

SequrityTests.java > SecurityTests.java