ReceiptsKeeper

Интересен взгляд профессионала со стороны. Что можно сделать более красиво и правильно, какие подходы использовать не стоит, как улучшить читаемость кода, возможные улучшения для recycler view, любые другие комментарии и замечания

vadiole

Вообще, на редкость аккуратный и приятный проект. Я даже звезду влепил.

Название репозитория, имя Gradle-проекта и заголовок в README нужно согласовать, они различаются одной буквой.

Начнём с :core

release {
    isMinifyEnabled = true
    proguardFiles(getDefaultProguardFile("proguard-android-optimize.txt"), "proguard-rules.pro")
}

Непонятно, зачем библиотечному модулю минификация. Если запустить ./gradlew :core:assembleRelease, получим аарник совсем без классов. Видимо, при сборке всего приложения minify к кору не применяется, раз к фатальным последствиям это не привело.

Крайне не рекомендую использовать «дефолтные» proguard-rules. Там очень много бездумных -keep, а некоторые оптимизации отключены, потому что Android 2.x не переваривал такой байт-код. Для примера можно ознакомиться с моим разбором других proguard-rules, идеи оттуда актуальны и здесь.

Каждый раз немного пугаюсь, когда нахожу consumer-rules: сразу хочется узнать, чего такого ужасного автор навертел. Но тут всё в порядке, файл пустой 😅

implementation("androidx.appcompat:appcompat:1.4.0")

Как показывает практика, при minSdk 21 он не очень-то и нужен. Если компат не тянется транзитивно, его можно выкинуть и сильно сэкономить на размере приложения. Конкретно в этом проекте компат нужен для core-ktx, но ktx обычно выкидывается крайне безболезненно.

override fun onClick(view: View) {
    if (enabled.getAndSet(false)) {
        view.postDelayed(ENABLE_AGAIN, intervalMillis)
        @Suppress("UNCHECKED_CAST")
        click.invoke(view as T) // as long as the onClick is set correctly - it is safe
    }
}

companion object {
    @JvmStatic
    private var enabled = AtomicBoolean(true)

    @JvmStatic
    private val ENABLE_AGAIN = { enabled.set(true) }
}

Здесь вижу несколько нюансов.

• Нет никакого смысла делать атомик, ибо все клики обрабатываются в UI-потоке. Джейк вот использует статическую переменную, например.
• ENABLE_AGAIN имеет тип () -> Unit. Каждый раз, когда такой объект передаётся в postDelayed, создаётся дополнительный объект-адаптер, который реализует Runnable и делегирует run() к invoke(). Чтобы этого избежать, нужно сразу присвоить Runnable в ENABLE_AGAIN, тогда тип сконвертируется непосредственно при компиляции.
• click.invoke(view as T): просто хочу сказать, что кастить вью к известному типу — это очень удобно. Я тоже так делаю.
• @JvmStatic существует для интеропа с джавой, он добавляет статические аксессоры, которые внутри себя лезут в компанион. Если же нужно, наоборот, избавиться от аксессоров, для этой цели существует @JvmField. Но в данном случае котлин понимает, что приватные свойства снаружи не видно, и сам их делает статическими и не генерирует аксессоров, так что ни @JvmStatic, ни @JvmField ничего не изменят.

var <T : View> T.onClick: (T) -> Unit
    get() = throw RuntimeException("There is no getter for onClick")
    set(action) = setOnClickListener(DebouncingOnClick(click = action))

Существует два разных способа избавиться от геттера: поставить на него @Deprecated(level = ERROR), чтобы получать ошибку компиляции, либо всё свойство заменить на метод. Оба подхода активно используются в Splitties Views DSL.

if (isEnabled) {
    isEnabled = onBackPressed()
} else {
    requireActivity().onBackPressed()
}

Обычно onBackPressed отвечает, обработан ли back, и если нет, то событие всплывает к родителю. Например, если фрагмент не обработал back, то надо попнуть бэкстэк, а если он и так был пуст, то закрыть активити.

Здесь же я вижу два отклонения от этого соглашения в случае, если onBackPressed вернёт false:

• в первый раз событие не всплывёт, а просто проглотится (и listener отключится),
• во второй раз событие вообще не попадёт в listener. Нам просто не вызовут handleOnBackPressed.

По сути достаточно вместо ифа написать onBackPressed(), тем более что false оттуда никогда не возвращается.

class Scale(...) : Visibility() {

Можно не писать собственный Transition, а использовать ObjectAnimator с уже готовыми свойствами View.SCALE_X, ~_Y. Либо через XML (FragmentManager умеет доставать из ресурсов как устаревший animation, так и animator), либо переопределив onCreateAnimator во фрагменте и вернув произвольный AnimatorSet с ObjectAnimator'ами внутри. (Но я так не пробовал, сужу чисто по исходникам.)

abstract class BaseViewModel : ViewModel()

Зачем? Не, я понимаю, что может появиться какая-то общая логика, но почему-то никогда не встречал, чтобы кто-нибудь делал, например, BaseTextView, BaseEditText, BaseButton и т. п. просто потому, что когда-нибудь оно может понадобиться.

interface Navigator {
    fun navigate(destination: String, args: Bundle? = null)
}

Вижу в таком подходе следующие фейлы:

• в качестве destination можно передать любую строку, в том числе неподдерживаемую (в первом приближении решается заменой на enum);
• сильное связывание: вызывающий знает некое имя/тэг/ключ вызываемого, определённый в активити, и умеет формировать аргументы для него, тем самым привязываясь и к родительскому активити, и к братским фрагментам;
• динамическая типизация: в качестве аргументов можно передать бандл с любым мусором.

Все эти недостатки, разумеется, унаследованы у самого механизма фрагментов, но их можно надёжно запрятать. Сам я когда-то типизировал фрагменты, но проект, для которого я это писал, ушёл от меня, и библиотека безнадёжно устарела.

Недавно подсмотрел у коллеги Алексея Долгого подход с координаторами, понравилось. Суть примерно такая:

class SomeFragment(private val coordinator: Coordinator) : Fragment() {

    ...

    interface Coordinator {
        fun toOther(arg1: String, arg2: Int)
        fun back()
    }
}
class SomeActivity : FragmentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        supportFragmentManager.fragmentFactory = FragmentFactory { loader, name ->
            when (loadFragmentClass(loader, name)) {
                SomeFragment::class.java -> SomeFragment(object : SomeFragment.Coordinator {
                    override fun toOther(arg1: String, arg2: Int) =
                        replace(OtherFragment::class.java, OtherFragment.args(arg1, arg2))
                    override fun back() =
                        onBackPressed()
                })
                OtherFragment::class.java -> OtherFragment()
                else -> throw AssertionError()
            }
        }
        super.onCreate(savedInstanceState)
        if (savedInstanceState == null) {
            TODO("add root fragment")
        }
    }
} 

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

Пробежимся по списку прошлых минусов:

• параметра destination больше нет, вместо него используются разные функции,
• вызывающий фрагмент ничего не знает о родственных активити и фрагментах и зависит только от координатора, который позволяет делегировать роутинг на уровень выше,
• сам фрагмент передаёт аргументы в функцию, а бандл формируется роутером, который точно знает тип фрагмента.

val Context.hasNetworkConnection: Boolean
    @SuppressLint("MissingPermission")
    get() {

Вместо @SuppressLint лучше просто перенести разрешение в манифест этого модуля. А вообще, вся эта длинная штука имеет смысл, когда приложение заодно подписывается на состояние соединения и умеет автоматически делать перезапрос. Вот пример оборачивания состояния в LiveData. Написан он плохо, но главное — общая идея.

Здесь же (дорогостоящая) проверка сети используется прямо перед запросом, хотя с таким же успехом можно просто обрабатывать IOException (а ведь его в любом случае нужно обрабатывать).

arrayOf(TRANSPORT_*) мог бы быть intArrayOf. Также на просторах интернета я нашёл более простой вариант: capabilities.hasCapability(NET_CAPABILITY_INTERNET).

Теперь :app, начну с ресурсов

activity_main — это два бесполезных контейнера. Наружный фрейм — без комментариев. Вместо внутреннего FragmentContainerView можно использовать фрейм, который всегда есть в активити: android.R.id.content. Тем более что функциональность FragmentContainerView не используется.

Все fragment_* содержат импровизированный тулбар, который можно вынести отдельно и вставить с помощью <include>. Вместо отдельной вьюшки-дивайдера можно тулбару поставить на фон такой <shape>, у которого внизу будет линия.

В fragment_details вместо fade подойдёт изкоробочный fading edge. В панель с кнопками напрашивается baselineAligned="false", иначе при переводе на языки, на которых текст на кнопке не влезает в одну строку, будет неприятный сюрприз.

Код :app

data, di, mapper, model, repository, ui, usecase, или как заблудиться в трёх экранах, не привлекая внимания санитаров. Здесь не буду растекаться мысию по древу, ибо уже писал об этом.

BaseConverter — это на самом деле BaseStringConverter. Наследуется от него аж один конвертер для LocalDateTime. Не вижу никакого профита от хранения даты строкой. Если хранить epoch second (long), то ничего не теряешь, зато можно построить индекс по дате и супербыстро искать записи за определённое время.

Также нет никакого смысла иметь отдельно ReceiptEntity, HistoryDomain.Receipt и ReceiptEntityMapper между ними (который ещё зачем-то доставляется диаем). Теперь добавление какого-нибудь поля стоит в три раза дороже. HistoryDomain.Receipt.url мог быть экстеншен-пропертёй, отдельный класс для этого не нужен. formatter здесь использует системную локаль, что может приводить к нежданчикам.

То же самое касается форматтера в ReceiptRawMapper. Также не вижу необходимости создавать Reader и нарезать текст на строки. Я бы сделал так:

val start = rawString.indexOf("Дата")
if (start >= 0) {
    var end = rawString.indexOf('\n', startIndex = start)
    if (end < 0) end = rawString.length
    datetime = LocalDateTime.parse(rawString.substring(start + 5, end), …)
}

Всю суть маппера можно перенести в одну функцию, findReceiptDateTimeOrElse(date: LocalDate): LocalDateTime. Fallback на now() может сбивать с толку, вместо этого я б сделал date.atTime(0, 0). А если считать, что транзакция ровно в 00:00:00 маловероятна, то такое время можно трактовать как «неизвестно» и не показывать его. (Для пущей надёжности можно разнести дату и время на две колонки: INT epochDay NOT NULL, INT secondOfDay.)

HistoryDomain дублируется ещё и HistoryItem'ом. Такая дополнительная сложность вызывает у меня искреннее недоумение. Не нашёл, чтобы у ReceiptRaw использовались equals, hashCode, toString, destructuring, copy, поэтому модификатор data нарекаю бесполезным.

Не вижу абсолютно никакого смысла в классе ReceiptRepository. У него нулевая смысловая нагрузка (зато благодаря выводу типов приходится угадывать, какой тип возвращают нижние два метода.

То же самое касается DeleteReceiptUseCase. Краткое и исчерпывающее содержание класса: ReceiptRepository::deleteReceipt.

override fun navigate(destination: String, args: Bundle?) {
    if (supportFragmentManager.findFragmentByTag(destination)?.isAdded == true) return

Вот это довольно стрёмно. В теории же могут быть сценарии, при которых разные экземпляры одного фрагмента добавляются несколько раз.

Насчёт ScannerView. color = -0x1 — а чего не Color.WHITE? Не каждый же с ходу догадается. :) Вместо четырёх идентичных фрагментов кода, формирующих уголки, можно сформировать один уголок в координатах (0, 0) прям в конструкторе, а при рисовании использовать аффинные трансформации, поддерживаемые канвой: translate, чтобы подъехать к месту рисования уголка, и scale на -1, чтобы уголок рисовался с отражением.

DetailsFragment. Большой жирный лайк за то, что и без хрома, и вообще без браузера в ОС приложение реагирует понятно и корректно! Так, а зачем приседания с инсетами? Можно же просто развесить fitsSystemWindows на некоторые контейнеры, не?

drawColor(BLACK, SRC_ATOP)
drawColor(Color.WHITE, DST_OVER)

Отличная находка! Только, это, либо заимпорти оба цвета, либо в обоих случаях пиши Color.~.

warmupCustomTabs уместно было бы вызывать из коллбэка didLoad (ты случаем не из айоса пришёл? :), иначе receiptUrl имеет все шансы быть null, следовательно, должного прогрева не произойдёт.

ReceiptCell (ну точно из айоса!): интересный подход, но у нас так не принято. За хранение отдельных вьюшек айтема должен отвечать вьюхолдер, он же при желании может заниматься байндингом.

onClick = { // onClick in init to avoid creating a new listener every bind
    receiptId?.let { it1 -> click.invoke(it1) }
}

it1 — плохое имя. Можно подавить внешний it (onClick = { _ ->), можно нормально назвать внутренний (?.let { id ->).

HistoryAdapter : RecyclerView.Adapter. А чем не подошёл ListAdapter?

stateRestorationPolicy = StateRestorationPolicy.PREVENT_WHEN_EMPTY — шикарно, запомню, спасибо.

HistoryFragment:

if (isGranted) {
    navigator.navigate(SCANNER_FRAGMENT)
} else {
    val canRequest = shouldShowRequestPermissionRationale(requireActivity(), CAMERA)

По идее, наоборот, нужно показывать rationale перед запросом разрешения, если shouldShow. А если разрешение уже отклонили, то, во-первых, shouldShow будет true, а, во-вторых, shouldShow ≠ canRequest. .dismiss() при обработке кнопок не нужен, диалог делает это сам. Поэтому для кнопки Cancel в качестве listener'а отлично подойдёт null.

HistoryViewModel:

.fold(firstHeader) { acc, receipt: HistoryDomain.Receipt -> // inserting date headers
    val lastReceipt = acc.last() as? HistoryDomain.Receipt ?: return@fold acc + receipt
    return@fold if (lastReceipt.date != receipt.date) {
        acc + HistoryDomain.Date(receipt.date) + receipt
    } else {
        acc + receipt
    }
}

?: return и else делают одно и то же, так что от labeled return тут несложно избавиться, перенеся всё в одно условие. Вообще, конструкция не только страшненькая, но и жутко неэффективная. Тут лучше создать ArrayList и добавлять элементы в него, ибо list + element влечёт за собой полное копирование списка.

И зачем создавать одни структуры данных, если потом они маппятся в другие? Можно же напрямую сложить в новый лист финальные данные, заменив fold+map на один проход.

ScannerViewModel:

saveUseCase.invoke(raw, id, date).fold(
    onSuccess = {
        _result.value = Result.success(id)
    },
    onFailure = {
        _result.value = Result.failure(it)
    }
)

Result разворачивается, чтобы потом его завернуть обратно, я всё правильно понял? :)

Хорошо бы принять одно соглашение об обработке ошибок: либо result складывать непосредственно во flow, либо выставлять значение в один flow из двух (и не забыть занулить второй).

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