search

kane2033 / WeatherKODE

0 stars 0 forks source link

CodeReview 2 #3

Open VadimMoschenkov opened 3 years ago

VadimMoschenkov commented 3 years ago

WeatherByCityDto.toWeather()

Метод конвертирует модель data -> domain при этом кидает исключение, но это выглядит немного странно т.к. на случай если данные отсутствуют например есть обработка и подставляется значение - WindDirection.NORTH. Поэтому поведение при проверке isNorth(degree: Int) которое бросает исключение не понятно. Кроме этого исключение нигде не обрабатывается. Как итог при конвертации вполне допустимо бросать исключения, но в данном случае есть исключение бросается только в одном случае и не понятно почему нельзя подставит дефолтное значение

NetworkAvailabilityInterceptor

Для чего нужен этот класс? ведь ретрофит самостоятельно может бросить исключение по которому можно понять что это ошибка сети. NetworkHandler - данный класс только проверяет включен ли интернет на телефоне - это можно использовать например для подсказки пользователю о том что вероятно у него на телефоне отключен интернет. При этом если сервер недоступен или в сети не доступен интернет то ошибка будет другая.

NetworkModule#provideOkHttpClient

Общий метод конструирующий OkHttpClient, при этом к нему добавляется OpenWeatherAuthInterceptor который актуален только для запросов погоды. В данном приложении так можно делать, но в целом добавлять такой ключ ко всем запросам не имеет смысла, поэтому нужно явно сконфигурировать koin (https://insert-koin.io/docs/reference/koin-core/definitions#definition-naming--default-bindings) что бы собирать OkHttpClient с нужными параметрами под ситуацию - в данном случае для вызова методов сервиса погоды нужно добавить OpenWeatherAuthInterceptor

MainActivity

Главная активити должна находиться в пакете presentation

kane2033 commented 3 years ago

WeatherByCityDto.toWeather()

Теперь функция конвертации всегда возвращает дефолтное значение WindDirection.North (8bf8028)

NetworkAvailabilityInterceptor

Добавил NetworkExtensions.executeSafely(), в котором в try происходит вызов call.execute(), а в catch IOException трансформируется в новый Failure.ServerFailure. Почему IOException? В документации Retrofit Call.execute():

@throws IOException if a problem occurred talking to the server.

Поэтому, IOException -> Failure.ServerFailure. NetworkAvailabilityInterceptor теперь возвращает Failure.NoInternet. Таким образом, теперь различаются ошибки со стороны сервера отличаются от ошибки об отсутствии интернета на устройстве. (71d9ba5)

NetworkModule#provideOkHttpClient

С помощью named(RETROFIT_AUTH), в WeatherModule теперь предоставляется Retrofit с OpenWeatherAuthInterceptor. Без указания named, будет возвращён стандартный Retrofit без дополнительного интерсептора. (28568c4)

MainActivity

Переместил в пакет presentation (было очень трудно). (f694567)