zhestovskyys / gitget Goto Github PK

Замечания по коду

Рум подключен, но я не заметил, чтобы он где-то использовался.

В дата-классах лучше не использовать изменяемые переменные, т.к. это нарушает принцип single responsibility. Если в дата-классе нужно изменить поле, лучше использовать метод copy.

Биндинг занулять не обязательно, его GC соберет автоматически. А вот адаптеры, декораторы и прочие лисенеры у ресайклер вью (и не только) нужно снимать, потому что из-за них течёт память.

Зачем тут кастомная фабрика, если у вью модели используется конструктор по умолчанию? Кастомная фабрика обычно используется, чтобы передать что-то вью модели в конструктор.

Излишне. Backing property используется для инкапсуляции данных внутри класса. Например:

        class SomeViewModel: ViewModel() {
            private val _viewState = MutableLiveData<ViewState>()
            val viewState: LiveData<ViewState> = _viewState
        }

Это делается для того, чтобы нельзя было менять viewState вне вью модели, т.е. вьюха может только подписаться на изменение viewState и как-то реагировать на изменение, а саму логику формирует именно вью модель.

!! (force unwrap) - зло! Не используй его пожалуйста! :)

Можно было обойтись без этого метода, подключив либу-делегат вью биндинга. Но это больше вкусовщина.

Излишние комментарии, не несущие никакой смысловой нагрузки.

Не вижу смысла использовать let, лишняя вложенность кода.

Лишние пустые строки. Если будет большой проект, где используется сторонний анализатор кода, например SonarQube, то он такое не пропустит.

Зачем эти переменные публичные?

Постфикc Impl тут явно лишний, потому что обычно он используется в том случае, когда репозиторий реализует интерфейс.

Зависимости со звездочкой тоже лучше не использовать. Сторонние анализаторы кода ругаются на это. Сокращение зависимостей можно вырубить в настройках студии.

Непонятная константа, еще и публичная. Константы вне класса можно объявлять только приватные. Публичные константы объявляются внутри класса в блоке companion object. Это связано с тем, что во время билда происходит излишняя генерация геттеров и сеттеров для констант, а это влияет на время сборки. На маленьких проектах это не совсем заметно, но на больших проектах каждая секунда сборки - на вес золота.

Почему здесь не использовал backing property?

get() не обязательно использовать в backing property.

Комментарии обычно используются для того, чтобы объяснить не совсем очевидное решение. Например, у тебя огромный проект, где намешана куча всего: java, kotlin, mvp, mvvm, syntetic, viewBinding и т.п., и тебе пришёл баг в очень старом коде, который непонятно как и кем написан, у тебя нет времени переписывать весь класс или даже весь экран, ты придумал какое-то быстрое, но далеко не идеальное решение. Соответственно, ты должен оставить комментарий, почему ты сделал именно так, чтобы на ревью не было вопросов, и была какая-то информативность для следующих разработчиков, или даже для самого себя, потому что скорее всего через месяц ты забудешь, почему ты это сделал. Вот так работают комментарии. Очевидные вещи комментировать не нужно.

Зачем в функции itemId? он не используется.

Пустой комментарий.

Зачем тут 2 лайаута? Лишняя вложенность.

Слишком длинная строка. Сторонние анализаторы кода ругаются на это.

Нельзя использовать match_parent внутри ConstraintLayout.

Общие замечания

• Излишнее использование Parcelable. Сериализацию лучше использовать по месту требования, не нужно использовать её просто так.
• Почему не использовал Dagger? Если возникли трудности, нужно разбираться, задавать вопросы.
• Есть несколько замечаний по неймингу ресурсов проекта, id вьюх, каталогов. Скоро доделаем документацию по неймингу, нужно будет её придерживаться.
• Репозитории сделаны как singleton, так делать нельзя.