РК2 - Githubissues

KSolovyev commented 8 years ago

~~Падают сервер и тесты~~

ERROR StatusLogger No log4j2 configuration file found. Using default configuration: logging only errors to the console.
Exception in thread "main" org.hibernate.internal.util.config.ConfigurationException: Could not locate cfg.xml resource [dbconfig.xml]
    at org.hibernate.boot.cfgxml.internal.ConfigLoader.loadConfigXmlResource(ConfigLoader.java:53)
    at org.hibernate.boot.registry.StandardServiceRegistryBuilder.configure(StandardServiceRegistryBuilder.java:163)
    at org.hibernate.cfg.Configuration.configure(Configuration.java:259)
    at main.Main.main(Main.java:37)

ValerGit commented 8 years ago

Все файлы конфигурации у меня вынесены в папку setups и отмечены в IJ как sources. Для запуска из IDE данное решение работает. Для деплоя позднее будет создана ресурсная система. Возможно ли такое решение? Или сейчас избавиться от конфигураций и логов?

KSolovyev commented 8 years ago

На счет некомпилируемости отбой: это что-то у меня локальное было

ValerGit commented 8 years ago

Зафиксила проблемы) Верное ли решение прокидывать HibernateException до main функции и даже не поднимать сервер без подключения к БД?

KSolovyev commented 8 years ago

Зависит от того можете ли вы работать без данной базы. Если нет, то можно обернуть HibernateException в LaunchException (класс, который придется написать), и в случае, если в main-e вы ловите его, писать, что все пропало, и завершать работу сервера, закрывая все ресурсы.

ValerGit commented 8 years ago

Исправила

KSolovyev commented 8 years ago

Нужно добавить в readme инструкцию "как запускать ваш сервер". Какие пакеты должны быть установлены, какие настройки должны быть сделаны, что выполнять в консоли.

ValerGit commented 8 years ago

Может быть update ?

Заменила на update в сеттингах mysql и добавила индексы. Считаю, что h2 стоит оставить в прежнем виде, тк данная БД используется только для запуска тестов

Остальное исправила

ValerGit commented 8 years ago

Done... =)

ValerGit commented 8 years ago

Будет ли верно уйти от атрибута isDeleted в сущности User и просто удалять пользователей из БД?

KSolovyev commented 8 years ago

Будет ли верно уйти от атрибута isDeleted в сущности User и просто удалять пользователей из БД?<

Подход с галкой верный. Его внедрение на более поздней стадии часто связано с громадными трудозатратами на выявление всяких багов. Думаю вы поняли почему =). На мой взгляд нужно запрещать регистрироваться пользователю с таким же именем, как и удаленного. Говорить, что имя уже занято.

ValerGit commented 8 years ago

Добавление пользователя с логином таким же как у удаленного вызовет ConstraintViolationException. Вам нужно подумать как вести себя с логинами удаленных пользователей.

Теперь логины и к имейлы уникальны. При изменении данных, пользователь сможет менять только логин и пароль.

Возвращаемый id - не очень хорошее решение. Это не очевидно и плохочитаемо. Надо вынести проверку уникальности в ресурс, а здесь просто сохранять. Признаком неудачи будет исключение. +

Вот здесь хорошо бы вызывать userService.checkLoginUniqueness, userService.checkEmailUniqueness

Заменила возвращаемое значение на boolean. Проверку решила не выносить в сервлеты буду проверять в методе save на уникальность. Мне кажется это более логичным.

Передача управления через исключение в 70 раз медленнее, чем через проверку условия. http://stackoverflow.com/questions/567579/how-expensive-are-exceptions Вместо throw Exception надо делать response.setStatus, responseBody.add и return

Все exceptions заменила на подобную конструкцию - вынесла ее в отдельный метод класса

KSolovyev commented 8 years ago

Заменила возвращаемое значение на boolean. Проверку решила не выносить в сервлеты буду проверять в методе save на уникальность. Мне кажется это более логичным.

Тогда подумайте вот о чем. Вам нужно сообщить пользоватлю, что он -ввел занятую почту -ввел занятый логин Как вы в сервлете поймете, что он ввел не так?

ValerGit commented 8 years ago

Константин, а можно сразу указать мне на все ошибки?.. Я же так и за год все не исправлю

KSolovyev commented 8 years ago

<Константин, а можно сразу указать мне на все ошибки?.. Я же так и за год все не исправлю

Есть несколько причин, почему ревью идет так, как идет. Во-первых я не бог, и не могу сразу охватить разумом весь проект и осознать все его недостатки и возможные места для улучшений. Я делаю это постепенно и когда у меня есть время. Предлагаю Вам сделать перерыв до завтра, я сегодня закончу ревью. Во-вторых ошибки тоже различаются. Есть те, которые приведут к багам, а есть те, которые просто ухудшают читаемость кода. Я их стараюсь давать поочереди. В-третьих ... Я понимаю вас. Ревью вызывает очень много злости, а некоторые замечания кажутся придирками и лишенными смысла (это не так! :) ). Сам процесс создания кода через ревью идет очень медленно, по сравнению с обычной работой, и это только добавляет. Мы занимаемся этим, потому что ревью - самый эффективный способ развития Вас как программиста. Ни просмотр чужих репозиториев, ни написание тонны кода, ни чтение умных книжек никогда не дадут Вам большего (зм.: отсюда не следует, что не нужно читать книжек и смотреть чужой код). Только переработка собственного кода ведет к развитию. Но обычно очень сложно понять, что же в нем не так. Поэтому радуйтесь, у меня получилось внимательно взглянуть на Ваш код и подсказать Вам места, где в нем что-то "не так". Остальным может так не повезти =) И есть еще одна мысль, которая может быть вас утешит: все оставшиеся 5 неприсланных ревью я буду смотреть так же =)

KSolovyev commented 8 years ago

Всё. Правьте и будем мержить

ValerGit commented 8 years ago

Удалила тесты БД - к ним требований в посте Егора не предъявлялось

Удалили метод doPut в SignUpServlet - наша игра не требует данного функционала, а фронтенд не требует четкого следования выданной документации

KSolovyev commented 8 years ago

Удалила тесты БД - к ним требований в посте Егора не предъявлялось

требование есть

Покрыть тестами AccountService. Все методы

Просто вы разделили его на два сервиса. Я написал, как сделать удобнее. Вы можете сказать мне, что не будете делать, потому что у вас нет на это времени =). Это валидный ответ. Я, если честно, не совсем понял, зачем вы их удалили. Они были хорошо написаны

KSolovyev commented 8 years ago

Удалили метод doPut в SignUpServlet - наша игра не требует данного функционала, а фронтенд не требует четкого следования выданной документации

Если фронтенду это норм, то и нам норм

ValerGit commented 8 years ago

Ввернула тесты

Это может быть id не того пользователя, что вы добавили

Ориентируюсь на уникальность имейла

KSolovyev commented 8 years ago

Ориентируюсь на уникальность имейла

Это инофрмация основана на знании внутренней структуры AccountService, которая в данных тестах и подвергается сомнению. Лучше написать

final UserDataSet user = new UserDataSet(...)
assertEquals(user.getId(), accountService.getUserByEmail(user.getEmail()).getId())

Ну и далее в таком духе

ValerGit commented 8 years ago

upd

KSolovyev commented 8 years ago

Окей, давайте замержим

java-park-mail-ru / 2016-02-unnamed-1-bmp

РК2 #9