Код ревью

[ashokolov]

1. В Gradle файле модуля App у тебя подключается либа retrofit. Это не правильно, эта либа должна находится только в data модуле. Чтобы нормально использовать при этом di, которая находится в app модуле, нужно в data модуле сделать retrofit creator, который будет создавать объект retrofit'а (в будущем его можно будет еще бафнуть, передавая разные http клиентов и т.п.) и уже его вызывать в koin'е.

2. Лучше версии библиотек хранить там же, где они и подключаются. Это позволит не переключаться между двумя файлами для обновления версии либы, а также уменьшит список в проектном gradle список, который может быть уж слишком большим и в котором можно будет "заблудиться"

3. Лучше ссылки на внешние api хранить в BuildConfig'е, просто задав их в gradle файле.

4. Если в liveDat'у пришла ошибка, то каждый раз, когда человек будет возвращаться на активность (к примеру перешел на другой экран и вернулся) или при смене конфигурации, будет вылетать ошибка (хотя ее на самом деле уже и нет). Для решения таких проблем используются Event'ы (подробнее о них можно почитать тут https://medium.com/androiddevelopers/livedata-with-snackbar-navigation-and-other-events-the-singleliveevent-case-ac2622673150)

5. Показывать snackbar когда все хорошо не всегда необходимо. Обычно, при успешном запросе просто показывается контент на экране, без перегрузки пользователя информацией о том, что все хорошо). Также прошу заметить, что если у человека возникла ошибка и он просто прождал несколько секунд, то snackbar с retry исчезнут, и попробовать перезапросить данные (к примеру, если инета не было, а потом появился) не получится уже никак

6. Комментарии лучше всего в коде писать на английском, такой общепринятый стандарт. И не нужно их так много. Обычно, если код хорошо написан, то он сам говорит о том, что тут происходит) Комментарии нужно оставлять только там, где может быть не совсем ясна логика происходящего, либо просто пояснить некоторые неочевидные моменты.

7. Вью должна быть максимально "тупой". В ней не должно быть обработки успешного или ошибочного кейса при взаимодействии с флоу. Обработка этого должна находиться во ViewModel, а в ней уже просто возвращать данные во фрагмент, где они должны быть отражены

8. Состояния фрагмента должны находится во viewModel'и. Так, даже при пересоздании фрагмента или activity все данные сохраняться и экран нормально после этого отобразиться. Поэтому состояния экрана по типу "идет загрузка, ошибка, контент" и т.п. должны быть именно во viewModel.

9. Общение между модулями должно происходить только с использованием интерфейсов (буковка L в SOLID'е). Так, можно будет поменять только класс реализации, передаваемый в необходимый класс, без внесения изменений в куче других мест. Сейчас же в твоей реализации UseCase'ы это просто отдельные классы, без наследования от какого-либо интерфейса. Взаимодействие domain и data слоя у тебя реализовано правильно

10. Директория util не совсем правильно названа. В ней у тебя находятся только функции расширения. Лучше ее тогда так и назвать extentions

11. CardView атрибуты для анимации лучше всего объявить как входные данные в функцию, потому что может быть ситуация, когда придется поменять, к примеру, длительность анимации. А чтобы их постоянно не писать просто задать начальные значения. И также их стоит вынести в отдельные переменные, так как они повторяются минимум 2 раза в этом файле

12. Android устройства бывают разные по размеру, поэтому НЕ векторные картинки нужно хранить в 6 форматах, чтобы они отображались "не шакально". Можно об этом почитать тут https://developer.android.com/studio/write/resource-manager

13. Если используешь ConstraintLayout, то лучше всегда вместо match_parent указывать 0dp. Очень редко, но бывают такие случаи, когда match_parent задает некорректное поведение у вью внутри ConstraintLayout.

14. domain модуль лучше классы разбивать на отдельные фичи, как это сделано в presentation. С ростом проекта будет все больше и больше useCase'ов, и отыскать нужные в одной директории будет невозможно

15. Строковые значения, такие как "temp, humidity, speed" и т.п. можно задать enum class'ом, где будет браться просто название класса с переводом его в lowerCase. Это позволит спокойно использовать этот класс и в других местах, если это понадобится, а так же соберет все воедино; не придется искать по разным классам такие строки и менять их

16. toBriefWeatherInfo примерно такой же подход, как и было описано выше, только наоборот. У enum class'а есть метод valueOf(String), который вернет enum объект по имени. Можно написать функцию, которая поменяет все пробелы на _ и переведет строку в старшему регистру. Это позволит при добавлении новых элементов добавить их только в enum class, а остальной код не трогать

[E-D-W-I-N]

Изменения уже в ветке master.

• 1. В data модуле создал RetrofitCreator, объект создаваемый им используется для DI.
• 2. Из проектного build.gradle файла убрал версии зависимостей которые специфичны только для одного конкретного модуля. Теперь там остались только версии зависимостей, которые используются в нескольких модулях сразу (чтобы не приходилось менять версии в каждом модуле).
• 3. Все ссылки на внешние API теперь находятся в BuildConfig.
• 4. Вместо Event для единовременных событий (показ ошибки) использую Channel, как рекомендовано в той статье, которую ты отправлял.
• 5. Убрал неуместные Snackbar'ы которые перегружали пользователя информацией. Также Snackbar с кнопкой Retry заменил на собственную CustomView. Изначально хотел использовать Banner из Material Design Components - https://material.io/components/banners, но оказалось что его реализации под Android еще нет (народ уже года 3 ждет). Пришлось сделать свою вью по их гайдлайнам. Может позже вынесу ее в отдельную библиотеку, наверняка пригодиться в будущем.
• 6. Убрал неуместные комментарии.
• 7. Теперь View обрабатывает только набор состояний, описанный в ее ViewModel. Вместо LiveData решил использовать StateFlow.
• 8. В каждой ViewModel описал набор состояний, который может иметь View. Каждое состояние отражает изменения, которые должна отобразить View при каком-либо из UseCase'ов.
• 9. Создал абстрактный класс UseCase от которого наследуются все существующие UseCase'ы. Именно этот абстрактный класс и используется теперь для DI.
• 10. Переименовал директорию util в extensions.
• 11. Изменил экстеншены для анимации CardView. Теперь в них можно передать начальную и конечную позицию анимации, а так же ее длительность.
• 12. Загрузил не векторные картинки в нужных форматах.
• 13. Теперь у вью внутри ConstraintLayout вместо match_parent используется 0dp.
• 14. UseCase'ы теперь разделены на отдельные фичи, как это сделано в presentation.
• 15. Создан enum для строковых значений.
• 16. toBriefWeatherInfo теперь использует valueOf для получения объекта из enum по имени.

[ashokolov]

1. Очень странно сейчас происходит работа с картой. При нажатии на карту, пин ставится не в место нажатия, а где-то неподалеку от места нажатия. При первом входе в приложение ставится пин, но не подгружается нижнее окно. Было бы приятнее, если бы пользователь заходит в приложение и ему сразу предлагалось посмотреть погоду по его местоположению. И анимации слишком медленные) лучше сделать их побыстрее.

2. Строковые переменные в AppModule нужно вынести в константные значения, так как они повторяются несколько раз в разных местах. Также AppModule можно сделать обычным object, тогда все, что связано с DI будет не просто в файле, а в отдельном объекте.

3. MaterialBanner Можно обойтись без приватных переменных с _. Это не имеет большого смысла, потому что для переменных класса в котлине автоматом генерируются геттеры и сеттеры, что уже позволяет удовлетворяет правилам ООП. binding лучше сделать val переменной, чтобы ее нельзя было переопределить.

4. MaterialBanner Лучше вынести в отдельную функцию места, где ты берешь из атрибутов значения

5. MaterialBanner Ты сделал расширение для всех View классов, хотя используется она только для MaterialBanner. Думаю лучше тогда и прописать этот метод от MaterialBanner класса

6. Если в функцию нужно передавать от 3 элементов и больше, то лучше передавать их задавая имя атрибута (к примеру showBanner(message = R.string...), так легче читать, а если придется поменять функцию, то не нужно будет думать какую переменную и в каком месте поменять, а просто дописать ее.

7. Можно не делать useCase.fold, а сразу при исполнении вызвать onSuccess/onFailure, так все будет выглядеть единообразнее

8. Для состояния лучше сделать обычный data класс, в котором будут возможные состояния (isLoading: Boolean, error: Throwable...). Твой подход тоже правильный, но при большом количестве состояний, тебе придется во всех when кейсах прописывать повторяющийся код (как это происходит в WeatherDetailsFragment), хотя с другим подходом достаточно в обзервере написать progressBar.isVisible = isLoading. Такую модель как у тебя лучше делать только для единоразовых эвентов.

9. weather_details_fragment.xml Лучше корневым элементов сделать FrameLayout. Он легковесней ConstraintLayout, а функционал констрейнтов тут и не нужен, достаточно указать гравити и марджины.

10. Лучше UseCase сделать интерфейсом, нежели абстрактным классом

11. Converter Обработка ошибки больше относится к domain слою, нежели к data. Если с сервера придет какой-то неправильный enum, то должна показаться ошибка, а не дефолтное состояние. Иначе это может привести в некоторое заблуждение, что произошла ошибка, но флоу все еще работает.

[E-D-W-I-N]

Изменения уже в ветке master.

• 1. Исправил взаимодействие с картой. Теперь при первом входе пользователю предлагается посмотреть погоду по его местоположению. Ускорил анимации.
• 2. Вынес строковые значения из AppModule в константы. Также сделал AppModule object'ом.
• 3. MaterialBanner Избавился от переменных с _ . Также сделал переменную binding val.
• 4. MaterialBanner Получение значений из атрибутов вынес с отдельную функцию.
• 5. MaterialBanner Функции скрытия и раскрытия теперь принадлежат исключительно самому MaterialBanner
• 6. Теперь при передаче в функцию 3-х или более элементов используются имена атрибутов.
• 7. Избавился от useCase.fold во ViewModel. Теперь сразу использую onSuccess/onFailure
• 8. Изменил подход к обработке состояний. Теперь взаимодействие между View и ViewModel происходит посредствам трех сущностей:

1. ViewState - состояние View
2. Action - единовременные события
3. Event - события, которые обрабатывает ViewModel после чего выдает соответствующий ViewState или Action

• 9. Корневым элементом weather_details_fragment.xml теперь является FrameLayout
• 10. Абстрактный класс UseCase переделан в интерфейс
• 11. Converter. Теперь если при обработке enum valueOf не найдет соответствующего элемента, то вернется null (раньше возвращался элемент DEFFULT). Этот null будет установлен как поле BriefWeatherInfo во WeatherDetails (потому что даже не смотря на то, что для BriefWeatherInfo не нашлось значения в enum остальные поля WeatherDetails все равно нужно отобразить в интерфейсе. Поэтому просто выкинуть exception нельзя). ViewModel при обнаружении в этом поле null выдаст Action во View (показ Snackbar'a).

[ashokolov]

1. MapFragment/WeatherDetailsFragment У тебя сейчас показываются ошибки, только которые ты ожидаешь (ошибки при работе с картами и т.п.). Желательно сделать еще обработку и всех остальных ошибок в else ветке, чтобы если придет какая-то неизвестная ошибка, это сразу можно было увидеть
2. MapFragment/WeatherDetailsFragment У тебя все еще используется when для твоего основного состояния в bindViewState. Если у тебя за раз изменится из разных мест состояние, то есть шанс упустить некоторые состояния. Лучше просто выставлять значения по типу viewState.fusedLocation?.let{/*YourCode*/} и также сделать с остальными атрибутами