МойПервыйПроект

Я понимаю, что мой код из разряда "быдлокод", т.к. это первая попытка написания приложения на андройд, поэтому чем больше критики и она подробнее , тем лучше. Ошибки, про которые я знаю: область объявления переменной NUM_COLLECTIONS и др, но при объявлении в классе Main, они не видны в других классах. Вопросы: что я хотел сделать, но не работает ни в каком виде - зациклить просмотр картинок (по настройке), как это сделать?

Zakirov Almaz

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

Приятно, что проект небольшой. Это очень хорошо: всего по нескольким файлам можно довольно быстро сказать о многом.

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

Начнём с Main. Кстати, из названия не угадаешь, что это — активити, фрагмент, меню, логика, всё сразу? Активити обычно так и называют: MainActivity.

//Эти переменные должны быть внутри Main, но тогда не получается
//к ним обратиться в ScreenSlidePagerAdapter, как это сделать?
var imageLoader: ImageLoader? = null
var CURRENT_PAGE = 1
var NUM_COLLECTIONS: Int? = null
var NUM_PAGES: Int? = null
lateinit var prefs: SharedPreferences

Это не так. ScreenSlidePagerAdapter — это inner class, из него видно все свойства активити, как свои. Проблемы начинаются в PageFragment, потому что фрагмент нельзя сделать inner. Из-за косяков, допущенных при проектировании андроида, фрагмент не может принимать зависимости в конструктор (чтобы исправить это, я когда-то подпёр фреймворк многочисленными костылями и разработал Flawless).

Затаскиваю свойства обратно в активити и чиню фрагмент:

--- app/src/main/java/com/example/childpikmain/Main.kt	(revision ce37277cd82e833a69bb2339db3231c8ae3e8776)
+++ app/src/main/java/com/example/childpikmain/Main.kt	(date 1600871789527)
@@ -23,20 +23,16 @@
 import com.nostra13.universalimageloader.core.ImageLoader
 import com.nostra13.universalimageloader.core.ImageLoaderConfiguration
 
-//Эти переменные должны быть внутри Main, но тогда не получается к ним обратиться в ScreenSlidePagerAdapter, как это сделать?
-var imageLoader: ImageLoader? = null
-var CURRENT_PAGE = 1
-var NUM_COLLECTIONS: Int? = null
-var NUM_PAGES: Int? = null
-lateinit var prefs: SharedPreferences
+class Main : FragmentActivity() {
+    //Эти переменные должны быть внутри Main, но тогда не получается к ним обратиться в ScreenSlidePagerAdapter, как это сделать?
+    var imageLoader: ImageLoader? = null
+    var CURRENT_PAGE = 1
+    var NUM_COLLECTIONS: Int? = null
+    var NUM_PAGES: Int? = null
+    lateinit var prefs: SharedPreferences
 
-class Main : FragmentActivity() {
-    /*var imageLoader: ImageLoader? = null
-    var CURRENT_PAGE = 1
-    var NUM_COLLECTIONS: Int? = null
-    var NUM_PAGES: Int? = null*/
     var btnSound: ImageView? = null
     private var buttonAnim: Animation? = null
     var buttonCheckAnim: Animation? = null
@@ -258,7 +254,7 @@
                 imageResource = Integer.valueOf(R.array.collection_2_pictures)
                 textResource = Integer.valueOf(R.array.collection_2_names)
             }
-            if (prefs.getBoolean("ShowImageText", true)){
+            if (PreferenceManager.getDefaultSharedPreferences(activity).getBoolean("ShowImageText", true)){
                 if (i == getResources().getStringArray(textResource!!.toInt()).size - 1) {
                     linearLayout.visibility = View.INVISIBLE
                 } else {
@@ -269,7 +265,7 @@
             }
             else{linearLayout.visibility = View.INVISIBLE}
 
-            val imageLoader: ImageLoader? = imageLoader
+            val imageLoader: ImageLoader? = ImageLoader.getInstance()
             if (imageLoader != null) {
                 imageLoader.displayImage(
                     "drawable://" + getResources().obtainTypedArray(imageResource!!.toInt())

Также здесь смешивается camelCase (imageLoader, prefs) и SCREAMING_SNAKE_CASE (NUM_COLLECTIONS, NUM_PAGES). Как правильно — разъясняет официальный кодстайл.

Аналогично здесь перемешаны nullable и lateinit. Многие из этих свойств вообще можно сделать локальными val внутри onCreate. (Эх, вот для джавы есть инспекция Field can be converted to a local variable.)

Вообще, значительная часть кода плохо отформатирована. Если этого не замечаешь, можно переформатировать весь файл (Code -> Reformat Code) либо поставить одноимённую галку в настройках коммита.

Теперь к инспекциям.

//эта аннотация и дает видимость во всех внутренних классах?
@Suppress("NAME_SHADOWING")

// не совсем понял, что дает аннотация Reycle и что она подавляет и стоит ли его использовать в конкретном контексте
@SuppressLint("Recycle")

//аналогично, не до конца понимаю нужна ли эта аннотация.
@SuppressLint("Recycle")

Как я и говорил, применять quickfix'ы можно только когда знаешь, что они делают. Ctrl+Click (Cmd+Click) по аннотации покажет её исходник, там же можно прочитать javadoc или KDoc:

/**
 * Suppresses the given compilation warnings in the annotated element.
 * @property names names of the compiler diagnostics to suppress.
 */
// ...
public annotation class Suppress(vararg val names: String)

/** Indicates that Lint should ignore the specified warnings for the annotated element. */
// ...
public @interface SuppressLint {

То есть первая аннотация подавляет предупреждения и ошибки компиляции котлина, вторая подавляет Android Lint. Эти аннотации не исправляют ошибки, они их убирают с глаз долой. Это оправдано лишь в тех случаях, когда инспекция слишком тупая и видит ошибку там, где её нет. Другими словами, в этом проекте Suppress'ы нужно убрать.

Почему-то многие интовые выражения завёрнуты в Integer.valueOf.

var valueOf = if (NUM_COLLECTIONS!!.toInt() == 1) Integer.valueOf(
    resources.obtainTypedArray(R.array.collection_1_namesound)
        .getResourceId(
            CURRENT_PAGE - 1, 0
        )
) else null
if (NUM_COLLECTIONS!!.toInt() == 2) {
    valueOf = Integer.valueOf(
        resources.obtainTypedArray(R.array.collection_2_namesound)
            .getResourceId(
                CURRENT_PAGE - 1, 0
            )
    )
}

Зачем? Кто это посоветовал? Почему? Я хочу объяснение! Нет совершенно никакого смысла приводить инт к инту. Аналогично с NUM_COLLECTIONS!!.toInt() — NUM_COLLECTIONS!! и так инт.

System.gc()

Здесь тот же вопрос — почему, зачем и по чьей наводке? Процитирую ключевое из javadoc к java.lang.Runtime.

/**
 * ... The virtual machine performs this recycling
 * process automatically as needed, in a separate thread, even if the
 * <code>gc</code> method is not invoked explicitly.
 * ...
 */
public native void gc();

То есть виртуальная машина сама запустит сборку мусора, когда посчитает нужным. Также имеет полное право проигнорировать вызов gc() на своё усмотрение.

Ещё один подозрительный кусок кода:

val arguments: Bundle? = getArguments()
val i: Int = arguments!!.getInt("position")
val i2:Int = arguments.getInt("collection")

Во-первых, getSomething в котлине можно заменять на something, о чём и подсказывает IDE.

Во-вторых, !! можно поставить сразу при присвоении, не создавая смарткаст.

В-третьих, сложно придумать имена для переменных хуже, чем i и i2.

Должно быть как-то так:

val arguments: Bundle = arguments!!
val position: Int = arguments.getInt("position")
val collection: Int = arguments.getInt("collection")

Анимации.

buttonAnim = AnimationUtils.loadAnimation(applicationContext, R.anim.button_anim)
rotate = AnimationUtils.loadAnimation(applicationContext, R.anim.rotate_circle)
buttonCheckAnim = AnimationUtils.loadAnimation(applicationContext, R.anim.button_check_anim)

Во-первых, нет никакого смысла загружать все анимации заранее. Их можно лениво загрузить по необходимости.

Во-вторых, многие анимации объявлены как сеты, хотя там всего одна анимация.

<set android:shareInterpolator="false"
  xmlns:android="http://schemas.android.com/apk/res/android">
    <alpha android:duration="50" android:startOffset="0" android:fromAlpha="1.0" android:toAlpha="0.5" />
</set>

Можно спокойно заменить на это:

<alpha
    xmlns:android="http://schemas.android.com/apk/res/android"
    android:duration="50" android:startOffset="0" android:fromAlpha="1.0" android:toAlpha="0.5" />

И, в-третьих, это очень старый API для анимации. Сейчас есть ObjectAnimator и View#animate, которые работают без XML.

Теперь переходим к меню.

try {
    handler = Handler(Looper.getMainLooper())
    handler!!.postDelayed({
        mpClear()
        mPlayer = MediaPlayer.create(this, R.raw.sound_menu)
        mPlayer!!.start()

    }, 0)
} catch (e: java.lang.Exception) {
    //errorShow("Main.playSound error - " + e.message)
    Log.i("Log", "Sound generator error - " + e.message)
}

Во-первых, try..catch обработает исключение, если оно вылетит из getMainLooper, конструктора Handler или метода postDelayed. А лямбда, переданная в postDelayed, вызовется асинхронно с другим стеком, и этот try..catch ничего не поймает.

Во-вторых, catch (Exception) ловит все возможные ошибки, даже всяческие NullPointerException, при которых лучше жёстко упасть, сразу же сообщая о поломке.

В-третьих, postDelayed(code, 0) эквивалентно post(code).

В-четвёртых, чтобы не писать !! на нуллабельной var, можно к выражению, создающему плеер, дописать хвост из .also:

mPlayer = MediaPlayer.create(this, R.raw.sound_menu)
    .also(MediaPlayer::start)

Единственное, чем этот код отличается от оригинала — start теперь выполняется раньше, чем присваивание. Но это ни на что не влияет.

try {
    handler!!.removeCallbacksAndMessages(null as Any?)
} catch (e: Exception) {

Видится мне, что, т. к. Handler иногда null, здесь периодически возникает NPE. Его не нужно ловить, его нужно не допускать:

handler?.removeCallbacksAndMessages(null as Any?)

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

Кстати, каст в null as Any? не имеет смысла, достаточно передать обычный null.

if (mPlayer != null) {
    mPlayer!!.stop()
    mPlayer!!.release()
    mPlayer = null
}

Здесь можно воспользоваться ?.let, чтобы выполнить код, только если плеер не null, а заодно захватить его в ненуллабельную локальную переменную:

mPlayer?.let { player ->
    player.stop()
    player.release()
    mPlayer = null
}

И снова вокруг этого кода имеется подозрительный try..catch.

Про вёрстку главной активити скажу лишь то, что мне подсказывает IDE: android:onClick="goChecking" работать не будет, т. к. соответствующий метод должен принимать View параметром. Вообще, не рекомендую использовать свойство onClick в XML, т. к. при инфлейте во фрагментах метод-обработчик всё равно ищется в активити.

Вёрстка меню. Просто посмотри на предупреждения в IDE.

Фрагмент с вьюпейджером.

RelativeLayout крут тем, что в нём вьюшки можно расположить относительно друг друга: прибить край одной вьюшки к краю другой, например. Здесь это не используется, Relative можно заменить на Frame, а layout_alignParentTop и layout_centerHorizontal — на layout_gravity="top|center_horizontal".

TextView, завёрнутый в LinearLayout, можно развернуть. layout_margin текствьюшки заменить на padding, чтобы под отступами оставался фон. layout_gravity, т. е. позиционирование в родительском контейнере, заменить на gravity, т. е. позиционирование текста внутри текствью.

Вёрстка settings_activity бесполезна. Кроме лишней вложенности здесь ничего нет.

Файл settings_activity.xml можно просто удалить, в SettingsActivity убрать setContentView(R.layout.settings_activity), а в .replace(R.id.settings, SettingsFragment()) поменять контейнер на android.R.id.content — это FrameLayout, который всегда есть в активити.

Файл с айдишниками — это категорически странно. Совершенно не понимаю, зачем вообще он существует.