Типичное Android-приложение: Dagger, RxJava, вот это всё.
Осваивая разработку под андроид, я прошел обучение на курсе от сообщества скилл-бранч. В конце обучения мы делали выпускной проект, который я писал на котлине. В дальнейшем на собеседованиях я показывал этот проект и мне часто говорили что я пишу на котлин как на джава что не коректно. Так же один мой знакомый показывал мой код тех лиду в Альфа Банке который так же подверг мой код определенной критике. Помогите разобраться, что же у меня не так в коде. Заранее спасибо.
Александр
Неидиоматичный Kotlin-код
companion object {
val INSTANCE = DataManager()
}
- Эта конструкция в Kotlin бессмысленна, т. к. есть object declarations.
- Классы в 📁 data.network.req и 📁 data.network.res умещаются в одну строку. Стоит сгрузить их в один-два файла: Kotlin, в отличие от Java, позволяет это сделать. Кроме того, классы, у которых всего одно свойство, обычно не нужны. Модификатор data, расставленный повсеместно, вызывает вопросы: действительно ли используются hashCode, equals, toString, copy, destructuring? Также у некоторых классов пустое тело — IDEA должна показывать warning, игнорировать их не стоит.
- Все свойства класса AlbumDto изменяемые. Предполагается мутировать объекты? Зачем? У всех свойств есть значения по умолчанию. Мне кажется, это достаточно вредная практика: можно создать объект, который не несёт вообще никакой информации. Лучше сделать всё неизменяемым и наполнять через конструктор. Кроме того, вторичный конструктор содержит множество неиспользуемых локальных переменных и ничего, кроме наполнения photoCards и дёргания геттеров, не делает — даже будучи очень невнимательным это легко заметить, т. к. в IDEA весь этот код помечается как неиспользуемый.
- Константы, объявленные непосредственно в классе, существуют для каждого экземпляра. Должны быть объявлены внутри companion (чтобы хранились в классе) и иметь модификатор const (чтобы инлайнились).
- for (item in album.photocards) photoCards.add(PhotoCardDto(item)) добавляет элементы в заведомо пустой список — стоит заменить на photoCards = album.photocards.map(::PhotoCardDto).
- Неверное использование non null assertion. App.rootComponent!!.inject(this) можно написать без !!, если добавить к свойству модификатор lateinit.
- Вместо бессмысленных non-null assertions можно написать, например, ProgressDialog(this).also { ... }. И вообще, использовать let, apply, also и т. п..
- if (supportActionBar != null) supportActionBar?.hide() — при наличии safe call проверка бессмысленна.
- builder?.cancel() — неправильное использование safe call. Стоит завести не-нуллабельную локальную переменную — тогда вопросительные знаки уйдут.
- Метод logOut, исходя из названия, обязан произвести некий сайд-эффект — разлогинить пользователя. Но столбики из вопросительных знаков означают, что, если mSharedPreferences == null, метод не выполнит свою работу. Такая «безопасность» только во вред: пусть лучше приложение упадёт и залоггирует ошибку, чем будет продолжать выполнение бессмысленных действий любой ценой.
- Нуллабельное меню в onPrepareOptionsMenu и нуллабельный holder в onBindViewHolder не имеют никакого смысла. Нужно убрать вопросительные знаки.
- Вся «безопасность» в holder?.tag?.text = context?.getString(R.string.hash_tag, list[position]) уродлива, бессмысленна и вредна: засоряет исходный код, генерирует лишний байт-код, ещё и помогает ошибкам спрятаться, выполняя бессмысленные действия там, где лучше было бы бросить исключение.
- Многие методы могли бы быть свойствами.
- if ((loginErrorHint.text == "" && loginErrorHint.text == "" && nameErrorHint.text == "" && .... Ужасно много скобок. Из == "" и .isEmpty() стоит выбрать что-то одно.
Coding Conventions
- Пробелы. if(it.code()==200) должно быть if (it.code() == 200);
fun getUserById(userId : String) : Observable<UserRes>{ должно быть fun getUserById(userId: String): Observable<UserRes> {. В проекте сотни несоблюдённых отступов. - Фигурные скобки. В {Observable.empty()\n} они не только лишние, но и расставлены так, чтобы сбивать с толку. Подобных мест превеликое множество.
- Десятки публичных полей. Большинство из них должны быть приватными. Обычно IDEA напоминает об этом.
Именование
- Про венгерскую нотацию я упомянул в предыдущем ревью. Здесь она тоже используется неправильно, но ещё и неконсистентно.
- Имена классов типа RealmManager, PhotocardRealm раскрывают детали реализации, но дают лишь малое представление о том, чем занимается класс. Нормальные версии названий: RepositoryForEverything, DataGod, RefactorMePlease; PhotoCard, PersistentPhotoCard.
- Имена интерфейсов, которые начинаются с I, отвратительны. Обычно интерфейсу и реализации можно придумать осмысленные названия — не ISomething и SomethingImpl, а, например, Table и InMemoryTable, SqliteTable; Document и ProtobufDocument, JsonDocument, XmlDocument. Если нормальные имена придумать не удаётся, то интерфейс лишний. Примеры хороших имён интерфейсов в JDK — CharSequence, List, Map, Lock.
- Дословно showLoad означает «показать нагрузку». Implements me означает «реализует меня». В RecentlyTagsAdapter recently — это наречие «недавно»: НедавноТэгиАдаптер. isValidateEmail дословно — «является ли проверить e-mail». Либо validateEmail — тогда от функции ожидаются сайд-эффекты, либо isEmailValid — тогда её нужно превратить в свойство. getAllCardFromRealm — «достать все карточка из Realm». Пока английский на таком уровне, стоит пользоваться онлайн-переводчиком при выборе имён.
- Переменная findFirstVisibleItemPosition называется так, как можно называть только функции — с глаголом в повелительном наклонении. Назначение метода, кстати, довольно сомнительно — в естественных условиях RecyclerView, у которого есть ID, сохраняет положение прокрутки.
- builder = AlertDialog.Builder(this).create(). Это не билдер, это AlertDialog.
- Ожидается, что менеджер чем-то управляет. Чем управляет ConstantManager? Почему одна константа — const, а остальные — нет? Общий класс с константами на всё приложение — плохая практика: сильно связывает разные модули, мешает понять, какие константы кому принадлежат.
- Из названий довольно сложно понять, чем isNetworkAvailable() отличается от isInternetAvailable. Та версия, что возвращает Observable, — сбивает с толку, т. к. я бы ожидал от него стрим булеанов, а по факту там Single.
Android
- Строки должны быть в ресурсах, не в коде и не в вёрстке. Хм, а для какого языка этот strings.xml? Откуда и зачем в русском языке слово «фуд»?
- val btn: Button = v.findViewById(R.id.sign_btn) as Button — и это при том, что в проекте есть ButterKnife и Kotlin Android Extensions. Одновременно.
- Кто будет управлять состоянием диалогов в RootActivity? Сейчас при повороте экрана диалог, скорее всего, будет исчезать, а в логах появится сообщение о leaked window. Обычно для уменьшения связанной с этим боли используют DialogFragment.
- notifyDataSetChanged спровоцирует байндинг всего списка заново. При добавлении данных нужно использовать notifyItemInserted.
- Удалять нужно лишние контейнеры, а не предупреждения о лишних контейнерах.
- Не вникал в подробности, но тут надо упрощать вёрстку.
- В большинстве файлов вёрстки ужасные отступы.
ReactiveX
- В RestCallTransformer используются fully-qualified имена классов. В Kotlin есть именованные импорты. Хотя я бы просто не использовал обе монструозных версии RxJava в одном проекте (id.zelory:compressor, например, есть и для второй версии).
- Вместо того чтобы использовать TextWatcherы, имеет смысл взять RxBinding.
- Все Observable в RestService стоит поменять на Single.
Тайпкастинг
- Метод changeScreen нужно вынести в интерфейс. Тайпкастинг противоречит ООП, т. к. эксплуатирует детали реализации. В хорошо спроектированном коде не встречается.
- CustomTextWatcher принимает два View, но сразу кастит их в конкретные типы. Тогда нет смысла расширять типы параметров до View, пусть они и будут EditText и TextView.
Рефлексия
Всё это весело и изящно, но по большей части бестолково, особенно после ProGuard.
String genericName = ((Class) ((ParameterizedType) screen.getClass().getGenericSuperclass()).getActualTypeArguments()[0]).getName();
- Этот код будет работать верно, только если класс экрана extends AbstractScreen. А, например, в ситуации, где SomeClass<T> extends CoolScreen<List<T>>, CoolScreen<T> extends AbstractScreen<Pair<String, T>> чёрт ногу сломит и нужны будут сложные решения вроде TypeToken.
- Использование аннотации вместо контракта:
get() {
val layout: Int
val screen: Screen?
screen = this.javaClass.getAnnotation(Screen::class.java)
if (screen == null) {
throw IllegalStateException("@ScreenAnnotations is missing on screen " + scopeName)
} else {
layout = screen.value
}
return layout
}
Итак, объект ищет на себе аннотацию, и, если её не окажется, бросает исключение (т. е. её наличие обязательно). Почему бы не использовать вместо этого базовые возможности языка и получить более простой и надёжный код?
abstract val layoutResId: Int
- В TreeKeyDispatcher, вероятно, ожидается, что inKey instanceof AbstractScreen — тогда можно просто вызвать getLayoutResId.
Screen screen; // зачем разделять объявление и присваивание?
screen = inKey.getClass().getAnnotation(Screen.class);
if (screen == null) {
throw new IllegalStateException("@Screen annotation is missing in screen " + ((AbstractScreen) inKey).getClass());
} else {
Ну и вообще, сложновато понять, что TreeKeyDispatcher делает. (Развешивает ключи по деревьям. Ваш Капитан.)
Другое
- Вместо .debugging(true) лучше использовать BuildConfig.DEBUG, чтобы в релизных сборках дебаг автоматически выключался.
- Каркас, который находится в master, кажется мне, во-первых, переусложнённым, а, во-вторых — скопированным откуда-то.
- Бессмысленное переопределение метода.
- Как я уже говорил, константы вроде BASE_URL стоит выносить в buildConfigField.
- «Авторизируйтесь что бы посмотреть данные профиля». авторизоваться — Викисловарь, «чтобы» или «что бы»?
Субъективщина
- Мне больше нравятся «пакеты по фичам», чем «пакеты по слоям». Сразу понятно, что делает код в отдельно взятом пакете.
- Мне не нравится Dagger. Он размазывает детали по «компонентам», внося неясность и повышая сложность.
- Как мне кажется, очень многие современные Android-разработчики знают Dagger (или что там сейчас модно? Toothpick?) и RxJava, но мало тех, кто в совершенстве знают свой основной и самый важный инструмент — язык программирования, а также своего коварного и хитрого напарника — Android.