Ревью

[E-D-W-I-N]

Добрый день.

Разделю ревью на какое-то подобие тем.

Начнем с простого - Code style:

1. Часто присутствуют вот такие моменты:

   private suspend fun getClosestRemoteKeys(state: PagingState<Int, News>): RemoteKey? {
       return state.anchorPosition?.let { position ->
           state.closestItemToPosition(position)?.let { news ->
               newsDao.remoteKeysId(news.title)
           }
       }
   }

В таких случаях можно обойтись без объявления возвращаемого типа и return. Получится что-то вроде этого:

private suspend fun getClosestRemoteKeys(state: PagingState<Int, News>) = state.anchorPosition?.let { position ->
        state.closestItemToPosition(position)?.let { news ->
            newsDao.remoteKeysId(news.title)
        }
    }

Подробнее тут.

2. NewsMapper.kt

   fun newsDataToEntity(articleNetwork: ArticleNetwork): News {
       val urlToImage = articleNetwork.urlToImage
       val title = articleNetwork.title
       val description = articleNetwork.description
       val dataPublishedAt = articleNetwork.dataPublishedAt
       val url = articleNetwork.url
       return News(title, description, urlToImage, dataPublishedAt, url)
   }

Можно было бы записать как:

fun newsDataToEntity(articleNetwork: ArticleNetwork) = with(articleNetwork) {
        News(
            title = title,
            description = description,
            urlToImage = urlToImage,
            dataPublishedAt = dataPublishedAt,
            url = url
        )
    }

Или вообще как Extension

fun ArticleNetwork.toEntity() = News(
    title = title,
    description = description,
    urlToImage = urlToImage,
    dataPublishedAt = dataPublishedAt,
    url = url
)

3. Наименование элементов верстки:

<androidx.recyclerview.widget.RecyclerView
            android:id="@+id/recyclerView"
            android:layout_width="match_parent"
            android:layout_height="match_parent"

<androidx.constraintlayout.widget.ConstraintLayout
        android:id="@+id/constraintLayout"
        android:layout_width="match_parent"
        android:layout_height="match_parent"

id у элементов должен отражать их назначение, а не быть просто копией типа элемента. Для примера recyclerView можно было бы назвать newsList или типо того. Почитай по различные подходы к наименованию элементов.

Общие моменты:

1. findViewById вещь крайне устаревшая и нерекомендуемая к использованию. Вместо этого лучше использовать viewBinding. Судя по подключенным библиотекам ты даже пытался это сделать (в build.gradle лежит implementation 'com.github.kirich1409:viewbindingpropertydelegate-noreflection:1.4.6'). Почитай поподробнее про viewBinding, вещь полезная и весьма удобная.

2. Реализация SplashScreen (которая с котиками) весьма странная, особенно учитывая то, что для Android 12 выкатили новое SplashScreen API которое позволяет удобно настаивать SplashScreen. Google крайне рекомендует мигрировать с реализаций вроде твоей на это новое API. К примеру у меня на телефоне Android 12 и твой SplashScreen у меня не работает вообще. Подробнее про SplashScreen API тут и тут

3. activity_news_main.xml на вершине иерархии содержит ConstraintLayout в твоем случае его можно легко заменить на FrameLayout который быстрее ConstraintLayout из-за меньшего вызова метода measure() на своих child-ах. Почитай про разные типы контейнеров и их отличия в плане производительности.

То же самое относится и к activity_web.xml

4. При клике на retryButton в NewsActivity логика весьма странная, вместо того, чтобы завершать Activity и стартовать заново лучше было бы повторять запрос данных, примерно как при SwipeToRefresh тогда пользователю не будет показываться анимация завершения Activity.

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

   

6. В проект подключено множество неиспользованных библиотек, например rxJava, хотя ты используешь Kotlin Coroutines. Наличие неиспользуемых библиотек замедляет сборку проекта. Лучше их убрать.

7. В NewsRemoteMediator можно обойтись без метода getLastRemoteKey. У state есть метод lastItemOrNull(), который фактически делает то же самое что и ты в своем методе.

8. В NewsRemoteMediator есть работа с бд -

   newsDao.deleteNews()
   newsDao.deleteAllRemoteKeys()

   и

newsDao.insertNews(articles)
newsDao.insertAllRemoteKeys(keys)

Такие вещи лучше оборачивать в транзакционные блоки withTransaction. Для этого нужно в конструктор NewsRemoteMediator передать объект NewsDatabase и вызвать у него метод withTransaction в котором уже работать с бд. А в идеале вообще не передавать NewsDatabase в конструктор, а внедрять его через DI, ну об этом ниже.

Замечания по исполнению задания:

1. Dagger весьма сложная вещь для освоения, особенно для начинающих. В задании не говорилось использовать конкретно Dagger, есть более простые варианты. Например библиотека Hilt, являющаяся обёрткой над Dagger и существенно упрощающая работу с DI. Еще есть Koin тоже весьма интересный варианты работы с DI. Можешь почитать про их отличия тут

2. Касательно проблем с RemoteMediator могу посоветовать в первую очередь жестко определить размеры изображений у элементов списка. Из-за того, что при загрузке списка картинок еще нет на экран вмещается много элементов и APPEND срабатывает ложно, также из-за этого сам список выглядит плохо, потому что все картинки разные по высоте. Также у Picasso можно добавить Placeholder при отрисовке картинки, так элементы будут изначально занимать нужный размер на экране, а после того как изображение загрузится, оно заменит собой плейсхолдер. Примерно как тут

Также можно поиграться с параметром initialLoadSize в PagingConfig.

Ну и на крайний случай можно отвязаться от получения последнего элемента из PagingState и получать его из локальной базы данных. Например как тут

[evtyryshkin]

Спасибо за Вашу обратную связь. Всё понятно и доступно, буду исправлять