Javanese Online

Основы?.программирования?.на?.Kotlin!!

Правильно ли реализован паттерн MVP ,правильно ли реализован mapping

Артем


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

Правильно реализовать MVP — это оксюморон. Я уже критиковал этот антипаттерн проектирования и рекомендовал статью про него. Думаю, суммарно сказано достаточно.

С маппингом та же ситуация: правильно реализованного маппинга не существует. Это избыточный императивный костыль, который показывает беспомощность большинства инструментов (де)сериализации или неумение ими пользоваться.

nullability

@SerializedName("id")
val id:Int? = null

Всё везде нуллабельное — это ужасно. nullability становится не фичей системы типов, а чудовищем, которое превращает приложение в недетерминированное месиво из вопросиков, готовое открыть пустой экран, сохранить невалидный объект, сделать любую глупость, лишь бы не упасть. Значения по умолчанию тоже легко использовать во зло: гостиница, у которой адрес «-», не имеет смысла и не должна существовать в базе данных, а уж тем более показываться в выдаче.


fun isConnectedToNetwork(context:Context?):Boolean?{

Принимать нуллабельный контекст бессмысленно: без него функция однозначно вернёт null, даже вызывать незачем. Возврат нуллабельного булеана, однако, не лишён смысла: если ConnectivityService не нашёлся, состояние сети определить невозможно.


filterSettings?.let { it ->
    mPresenter.setFilterSettings(it as FilterSettings)
}

Имеет ли смысл этот экран без FilterSettings? Если нет (да и фабрика со страшным названием getNewInstance принимает не-нуллабельный фильтр), зачем это разрешать и мучиться с нуллами, обмазывая всё вопросиками? Та же проблема и у другого фрагмента. И вообще, он подозрительно похож на первый.


(targetFragment as? MainScreen)?.handleFilterResult(filterSettings)
activity?.supportFragmentManager?.popBackStackImmediate()

Когда всё летит под откос, нужно падать. Тогда тестирование сможет быстро найти проблему, а ты увидишь стектрейс, который покажет на конкретную строчку. Вместо этого в экстремальной ситуации фрагмент ничего не делает, а пользователь видит, как на его действие нет никакого ответа. Потом одну звезду, конечно, поставит.

Вот ещё один пример:

context?.startActivity(intent)

(activity?.application as YHotelsApp).component.inject(this)

А если activity == null || application !is YHotelsApp, что тогда? Зачем игнорировать проблему и валиться в следующих строках с неинициализированным презентером, если можно падать сразу, используя non-null assertion, и узнать о проблеме раньше и точнее?


view?.nameText?.text = hotelDetails.name
view?.addressText?.text = hotelDetails.address
...

можно исправить на

view?.run {
    nameText.text = hotelDetails.name
    addressText.text = hotelDetails.address
    ...
}

но вообще в контексте этих повсеместных вопросиков хочется спросить: если ты без падения обрабатываешь ситуацию, когда view == null, почему при этом преспокойно позволяешь себе терять данные?


imageView.layoutParams?.height = ConstraintLayout.LayoutParams.WRAP_CONTENT
imageView.layoutParams?.width = ConstraintLayout.LayoutParams.WRAP_CONTENT
val params = imageView.layoutParams as ConstraintLayout.LayoutParams
params.setMargins(0,pictureTopMargin,0,0)

Снова вопросики безо всякого смысла: сначала ты проигнорируешь null, но уже в третьей строчке сделаешь cast, который требует, чтобы объект не был нуллом. Почему же не сделать каст сразу, а потом присваивать значения их первых двух строк?


itemView.itemInfoOnSuites.text = hotel.suites?.size.toString()

Не стоит удивляться, если количество доступных комнат в отеле — null. В этом месте используются заклинания для расширения пространства, благодаря чему там вполне можно уместиться.


А вот DiffUtilCallback почему-то игнорирует, что всё может быть нуллабельным, и резко упадёт, если имя или адрес отеля окажется нуллом.

Нужно в одной идее идти до конца: либо всё опциональное (что ужасно), либо всё строго, а падает сразу — на парсинге.

Избыточность

Дублирующимся buildConfigField место в defaultConfig.


@SerializedName переименовывает поле. Запись @SerializedName("id") val id не несёт дополнительного смысла в сравнении с val id. (Хотя может служить для большей явности и показывать намерение сериализовать поле, так что ошибкой это тоже нельзя считать).


Два одинаковых интерфейса и бессмысленная реализация, которая просто служит адаптером между ними, да ещё и создаёт по HTTP-клиенту на каждый вызов.

Кстати, ApiInterface — такая же тавтология, как и IT-технологии. Интерфейс должен описывать, за что отвечает его реализация (Hotels, например), а то, что он интерфейс, мы и так знаем.


@Module class DataSourceModule {

    @Provides @Singleton
    fun provideMainContentDataSource(): MainContentDataSource =
            RestDataSource()

}

Этот пример показывает, насколько же Dagger чудовищный и многословный. Вместо однострочного ::RestDataSource нужно написать целый класс с единственным методом из одного выражения и облепить аннотациями. Ради чего? Остальной пакет DI игнорирую, там нечего смотреть, т. к. он существует только для обслуживания даггера.


Не понял, зачем нужны интерфейсы FilterScreenContract, HotelDetailsScreenContract и MainScreenContract. У них по одной реализации.

Mapper

class Mapper не имеет смысла. Mapper().hashCode()? Класс и компаньон бесполезны, их можно убрать, вытащив функции на верхний уровень.


Единственный повод для существования функции, которая перекладывает значения полей из одного объекта в другой — это парсинг доступных номеров в гостинице из строки в список интов:

private fun getSuites(availableSuites:String?):ArrayList<Int>{
    val receivedString = availableSuites?.split(":")?.map { it.trim()} as ArrayList
    receivedString.remove("")
    val finalResult = ArrayList<Int>()
    for(i in receivedString){
        finalResult.add(i.toInt())
    }
    return finalResult
}

Его стоит переписать:

Переписываю в первый раз, «втупую»:

private fun getSuites(availableSuites: String): List<Int> =
        availableSuites
                .split(":")
                .map(String::trim)
                .filter(CharSequence::isNotEmpty)
                .map(String::toInt)

можно сделать проще и эффективнее. Переписываю во второй раз:

private fun getSuites(availableSuites: String): List<Int> =
        availableSuites.split(":").mapNotNull {
                it.trim().takeIf(CharSequence::isNotEmpty)?.toInt()
        }

private fun getImageUrl(imageId:String?):String{
    val imageBaseUrl = BuildConfig.IMAGE_URL
    val urlBuilder = StringBuilder()
    urlBuilder.append(imageBaseUrl).append(imageId)
    val imageUrl = urlBuilder.toString()
    return imageUrl
}

Заодно перепишу getImageUrl в соответствии с документацией, и, опять же, не надо принимать нуллы: уверен, урл https://github.com/iMofas/ios-android-test/raw/master/null ты получить не хочешь.

private fun getImageUrl(imageId: String): String =
        BuildConfig.IMAGE_URL + imageId

mapHotelDetails — очередная странная функция, которая зачем-то принимает нуллабельный параметр. Конструкции вроде

hotelDetailsDTO?.name?.let {
    hotelDetails.name = it
}

можно посыпать ?:, сократив каждую до одной строки, но нормальное решение — это, конечно, выбрать десериализатор, который в состоянии подставлять значения по умолчанию.


imageIsAvailable просто говорит о том, что урл не пустой. В этом флаге нет никакого смысла, ведь у нас есть сам урл и функция CharSequence.isNotEmpty().


Переписываем mapHotelsList с Java 6 на Kotlin:

fun mapHotelsList(hotelsDTO: List<HotelDTO>): List<Hotel> =
        hotelsDTO.map(::mapHotel)

Теперь самое главное. Правки, которые я предложил, однозначно стоит применить, но этого недостаточно. Маппер нужно полностью убрать. Несколько осмысленных строчек кода, которые там останутся, нужно преподнести десериализатору так, чтобы

Вижу, что в проекте используется Gson. Как бы порочен он ни был, на поля можно навешивать аннотацию JsonAdapter. После правильного её использования функции маппинга исчезнут за ненадобностью.

Тут хочется упомянуть, что аннотации — ущербный подход в принципе. Чтобы задать адаптер для поля, нужно указать его класс в аннотацию. Это обязывает к наличию конструктора по умолчанию, то есть такой адаптер не может иметь параметров.

Разное

RequestCode должен передавать вызывающий, т. е. активити или фрагмент. Компоненты, которые пытаются придумать оригинальный код, рано или поздно столкнутся друг с другом в попытке обработать один и тот же код.


Это попытка написать на котлине Java-код. Рекомендую ознакомиться с документацией.


var mPresenter — mМусор. Не надо mТак. А использовать эту нотацию неконсистентно, как в этом проекте, — ещё хуже.


Два очень похожих участка кода, и оба приделывают чекбоксам поведение радиокнопок. Нужно просто переехать на RadioGroup.


Плохая идея использовать lateinit как nullable. Две проверки isInitialized — это вполне себе звоночек. Вообще, все эти переходные состояния вроде mInitialInitializationIsDone просто не должны существовать. Передавай объекту в конструктор нужные данные, и пусть там всё инициализируется раз и навсегда.


Не понимаю, зачем презентер чистит состояние перед тем, как с его экрана уйдут. Он ведь после этого перестанет существовать, нет?


Тут определённо стоит почитать, что такое unchecked exceptions, NPE в частности, и почему их не стоит ловить. Выражение view?.hotelImage!! очень нездоровое. Похожий пример; ещё один пример нездоровой ловли исключений.


HotelDetailsScreenContract — ужасное название интерфейса. Мы и так знаем, что интерфейс == контракт == протокол, не нужно повторяться.


Этот код, дословно:

Про другой презентер писать не стану, там те же ошибки.


Splash придумали, когда не смогли стартовать приложение за приемлемое время. Какой-нибудь зелёненький банк может себе это позволить, а вот от тебя пользователи имеют возможность уйти. Визуально подсластить старт можно плейсхолдерами, т. е. показав, что уже всё почти готово, вот ещё немного, и всё будет.


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


Выполнять очистку в открытой для переопределения функции опасно. Тут либо @CallSuper, либо сделать функцию нормальной, финальной, выполнить очистку, а затем позвать переопределяемую функцию:

fun releaseView() {
    job.cancelChildren()
    this.view = null
    onViewReleased()
}
open fun onViewReleased() {
}

В адаптере не хватает notify. Это значит, что нужно перечитывать javadoc адаптера и экспериментировать до полного понимания происходящего.


Вёрстки на основе констреинта я пропустил. Без превью ничего не понятно, сплошное месиво. Всего пара наблюдений:


Fabric — это полотно. А фабрика — factory.

Вердикт

Сейчас я вижу, что ты пишешь код так, чтобы он как-нибудь да скомпилировался, без понимания того, почему компилятор не позволяет писать те или иные конструкции. Стоит отодвинуть в сторону архитектуры, паттерны, андроид и UI и поиграться напрямую с языком программирования. Сначала прочитать документацию целиком и пройти обучалку «Kotlin Koans». Затем, когда пишешь код, к каждому выражению, к каждой строчке нужно задавать вопрос: что будет, если сюда попадут такие-то данные? Как этот код работает? А как должен работать, если происходит то-то и то-то? Насколько код здесь хорош, понятен и выражает мою идею? Можно ли сделать лучше?

Затем, с полным пониманием того, как работает та или иная языковая конструкция, какую проблему решает и почему придумана, можно идти переписывать проект. Безо всяких MVP или Dagger. Так, чтобы хорошо работало. На любых входных данных, при разрывающемся интернете, в град, гололёд и даже при поворотах экрана.

Потом можно пробовать на вкус паттерны.

Javanese.Online в GitHub

Чаты и каналы в Telegram

RSS-лента