search

vsklamm / CppQuiz

Android app for cppquiz.org with the offline mode and other features
http://cppquiz.org/
GNU General Public License v3.0
6 stars 2 forks source link

Review 2 #14

Open sandwwraith opened 6 years ago

sandwwraith commented 6 years ago

Детально я всё не вычитывал (может потом ещё на это время выделю), пока в общих чертах/что сильно бросилось в глаза:

  1. OkHttp крута, но можно взять сразу Retrofit, чтобы самому не париться с конверсией, построением запроса и т д. (ещё оттуда сразу можно получать RxJava классы, чтобы не париться с тредингом и выкинуть асинк таски)
  2. У QuestionDao почему-то часть методов реактивная (с Single), а часть блокирующая
  3. https://github.com/vsklamm/CppQuiz/blob/199335d0868b1a433f0f414259efe287b1c57174/app/src/main/java/com/vsklamm/cppquiz/loader/DumpLoader.java#L68 Здесь во первых можно взять сразу .source() вместо того, чтобы InputStream потом в него 10 раз заворачивать (т.к. moshi и OkHttp из одной компании они подогнаны друг под друга хорошо), а ещё, как верно подсказывает идея, response body может быть null если что-то пошло не так.
  4. Всё ещё есть какие-то странные Serializable синглетоны со стейтом и сохранение данных в шаредпрефы в жсон. Ты придумал, в какую сторону будешь от этого избавляться?
  5. Куча (видимо пока) неиспользуемых презентеров
vsklamm commented 6 years ago

@sandwwraith

  1. Блокирующие юзаю не в UI потоке или вообще не юзаю 4 и 5. Да, это уйдет с MVP