e-d-w-i-n / contacts Goto Github PK

1. Дизайн слишком отличается в некоторых моментах. Желательно чтобы дизайн тех частей, что есть на макете соответствовал и в приложении. Особенно не забывай обращать внимания на бросающиеся мелочи (цвет текста, отступы линий и т.п.)

2. Заставлять пользователя заполнять все поля не очень юзер-френдли подход. Вспомни, что ты сам обычно заполняешь, чтобы добавить новый контакт в телефон?)) Думаю основными для заполнения должны быть только имя либо фамилия и номер телефона. Остальное можно опциональным сделать.

3. Внимательнее смотри на элементы пользовательского интерфейса: если нажать на стрелочку в выборе рингтона, то окна с выбором не появился. Появляется оно только с нажатием на само поле

4. У тебя в di находится AppModule. По сути вся di и рассчитана на то, что оно занимается неким объединением всего кода, поэтому предлагаю переименовать директорию в app и перенести туда ContactsApplication

5. Раз объекты из constants используются только в AppModule, то почему бы не перенести их туда и не сделать приватными? Также константные значения пишутся капсом и через нижнее подчеркивание

6. AppModule предлагаю разбить на 2 отдельных объекта. В одном будут просто factory и single элементы, во втором же будут только viewModel. Такое разделение немного сократит код в одном большом файле и чисто логически разделяет использование

7. EnumSafeValueOf лучше extentions называть так, для какого класса они используются, чтобы в него можно было добавить какие-либо еще функции, а не городить еще отдельный файл. Предлагаю переименовать в EnumExtentions. С другими экстеншенами предлагаю сделать аналогично

8. Fragment.makeDialog для диалоговых окон нужно использовать их Fragment версию. Делается это для того, чтобы при пересоздании экрана все осталось также, как и было. В данном подходе dialog просто исчезнет. Предлагаю логику создания диалога перенести в отдельный класс FragmentDialog.

9. Слишком большие функции onViewCreated и onCreateView. В большинстве случаев можно конечно поставить пустые строчки, чтобы было удобнее читать, но лучше все же вынести в отдельные функции, чтобы точки входа были максимально короткими

10. Почаще разделяй пустой строчкой некоторые блоки кода. Часто вижу в коде как код идет сплошняком, хотя чисто логически начался другой блок логики

11. Давай придерживаться одного формата структуры класса: сначала идет компаньон (при наличии), потом публичные переменные, приватные, необходимые override функции (как для activity и фрагментов например точки входа), публичные функции, приватные и override функции от реализуемых интерфейсов. Пройдись по всему проекту и поправь, где это не так

12. ContactsListItemDecorator встречаются магические константы (8f например), а также функция onDraw достаточно большая. По идее при onDraw может понадобится что-то еще делать, поэтому предлагаю текущий код вынести в отдельную функцию внутри класса.

13. ContactsListFragment: (activity as MainActivity).supportActionBar?.setDisplayHomeAsUpEnabled(true)

Такой код немного рушит концепцию фрагментов. По сути у тебя этот фрагмент жестко закреплен за твоей MainActivity. Если ты попытаешься использовать этот фрагмент в другой активности, то приложение просто упадет. Реализация фрагментов в идеале вообще не должна знать к какой именно активности она прикреплена. Чтобы избежать такого можно, к примеру, приводить не к MainActivity а к типу активности, которая используется в проекте (в твоем случае это AppCompatActivity). Прям идеальное решение это сделать интерфейс, который должна реализовывать активности и вызывать его в этом месте.

14. ContactsListAdapter Лучше объект DiffUtil поместить в компаньон адаптера

15. GlideImageLoader Лучше сделать объектом, так как каждый раз создавать его это лишние трудозатраты, а загрузка картинки может понадобиться часто. Также говоря о структуре проекта: не очень хорошо, когда классы просто так валяются в общей директории (исключением может быть разве что MainActivity, но и то на больших проектах ее помещают в определенный флоу). Предлагаю создать отдельную директорию common и поместить GlideImageLoader туда. Также директорию extentions тоже положить в нее, так как она относится только к presentation слою. Учитывая замечание выше у тебя получится 2 директории app и presentation, где в первой все для DI, а во второй уже presentation логика твоего проекта

16. ContactsListViewModel Достаточно прикольно, что ты реализовал смену темы приложения, но сейчас она доставит тебе немного проблем) Дело в том, что в этом классе ты напрямую обращаешься к элементу из data слоя. По сути это не запрещается согласно чистой архитектуре, но в твоем подходе немного смешивается логика: тема приложения это, по сути, полностью presentation логика. Но вот добавление смены темы с ее сохранением уже рассматривается как отдельный значимый флоу. В таком случае необходимо описать и бизнес логику твоего флоу. Поэтому лучше все же сделать отдельный юзкейс на получение и сохранение темы приложения. Также такой подход избавит тебя от лишнего lateinit во вью модели

17. ContactsListViewModel#obtainEvent Тут ты также напрямую обновляешь данные обращаясь к data слою. Тебе нужно в presentation подписаться на флоу из domain слоя, и он уже берет данные из data слоя. При обновлении той же сортировки ты можешь либо подписаться на другой флоу (что не очень правильно), либо просто при вызове функции менять параметр по которому сортируешь. Опять же: это значимый кейс для пользователя и он должен проходить через доменную логику приложения.

18. Чтобы не городить большую колбасень из UseCase<Flow<Result...дальшеленьписать>> достаточно сделать для каждого юзкейса свой интерфейс и чтобы он уже наследовался от колбасени выше. Таким образом ты только один раз напишешь её, а работать будешь уже с производным интерфейсом.

19. ContactsListFragment#contactsListAdapter тут ты в фрагменте явно задаешь куда нужно будет перенаправиться, так делать неправильно. Перенаправлением на другие экраны должен отвечать родительский элемент, в твоем случае это активность. Для этого просто во фрагменте вызываешь колбэк лисенера, который должна реализовывать активность и уже в активности делаешь переходы куда тебе нужно. Иначе у тебя фрагмент становится непереиспользуемым.
    UPD: посмотрел по другим фрагментам -- такая же ситуация, нужно исправить

20. ContactDetailsFragment хранишь значимую информацию внутри фрагмента contact, хотя всю информацию нужно хранить во вью модели. Так ты гарантируешь, что ничего не потеряется при пересоздании фрагментов.

21. Создание интентов для получения номеров, открытия браузера и т.п. лучше вынести в отдельный объект утилиты IntentUtil, и вызывать уже метод этого объекта

22. Вся логика должна находится внутри вью модели. Того же касается и валидации. Поэтому валидацию полей нужно перенести из фрагмента во вью модель

23. ViewMode Название класса очень путает (поначалу подумал что просто ViewModel не дописал)), лучше переименовать в ViewState или FragmentState

24. Обычно не используются репозитории. Чаще всего юзают DataSource (у репозитория немного другая логическая задача). И если делать уже как DataSource, то в реализации не должно быть никакие try catch (только в очень редких случаях, когда это необходимо на проекте, но это не твой случай). Вся обработка должна быть в domain слое.

25. У тебя в юзкейсах повторяется один и тот же код. Его можно добавить как базовый в run функцию твоего юзкейса, просто чтобы передавался блок кода, а сам перехват уже был зашит в run. И если нужно будет вызвать функцию без этого шаблона, можно создать какую-нибудь функцию invoke

26. data и domain слои: лучше расположение файлов сделать такое же, как и в presentation слое. То есть нужно сделать расположение по флоу, а не по usecase, model и т.п. Это пока что приложение достаточно маленькое, если оно станет больше, то ты просто не сможешь найти нужный юзкейс в директории с 150+ файлами юзкейсов)

В целом достаточно хорошо всё реализовано и фича со сменой темы хороша) А айтем декоратор для букв вообще пушка