Code clarity

[blueglyph]

This book and the source code need a serious rework to improve the clarity!

A few examples for the code.

Chapter03/Exercise3.03/app/src/main/java/com/example/dualpanelayouts/ListFragment.kt, onClick() function:

v?.let { starSign ->
            starSignListener.onSelected(starSign.id)
}

Don't use obscure "clever" code, if you test for null, make it explicit:

if (v != null) starSignListener.onSelected(v.id)

See how the code is more understandable, and actually shorter? Chances are it will also run faster, though here it'll be negligible.

One function above, onViewCreated(),

        val starSigns = listOf<View>(
            view.findViewById(R.id.aquarius),
            view.findViewById(R.id.pisces),
            view.findViewById(R.id.aries),
            view.findViewById(R.id.taurus),
            view.findViewById(R.id.gemini),
            view.findViewById(R.id.cancer),
            view.findViewById(R.id.leo),
            view.findViewById(R.id.virgo),
            view.findViewById(R.id.libra),
            view.findViewById(R.id.scorpio),
            view.findViewById(R.id.sagittarius),
            view.findViewById(R.id.capricorn)
        )

        starSigns.forEach {
            it.setOnClickListener(this)
        }

Oof. Try this instead:

        val starSigns = listOf(
            R.id.aquarius,
            R.id.pisces,
            R.id.aries,
            R.id.taurus,
            R.id.gemini,
            R.id.cancer,
            R.id.leo,
            R.id.virgo,
            R.id.libra,
            R.id.scorpio,
            R.id.sagittarius,
            R.id.capricorn
        )
        for (id in starSigns)
            view.findViewById<View>(id).setOnClickListener(this)

Of course, it would be even cleaner if those views were generated programmatically, it makes little sense to have to type all that manually, and it's prone to errors.

Another example, in Chapter03/Exercise3.02/app/src/main/java/com/example/fragmentintro/CounterFragment.kt, onViewCreated() function:

        val counter = view.findViewById<TextView>(R.id.counter)

        view.findViewById<Button>(R.id.plus).setOnClickListener {
            var counterValue = counter.text.toString().toInt()
            counter.text = (++counterValue).toString()
        }

        view.findViewById<Button>(R.id.minus).setOnClickListener {
            var counterValue = counter.text.toString().toInt()
            if (counterValue > 0) counter.text = (--counterValue).toString()
        }

You obviously work with an Int, not a String. The String is only the representation to display the value on the screen, don't use it to actually store the value, it's bad practice. Add a local variable that clearly represents the state.

     var counter = 0

...

        val counterView = view.findViewById<TextView>(R.id.counter)

        view.findViewById<Button>(R.id.plus).setOnClickListener {
            counterView.text = (++counter).toString()
        }

        view.findViewById<Button>(R.id.minus).setOnClickListener {
            if (counter > 0) counterView.text = (--counter).toString()
        }

Also, you'd benefit from using bindings, instead of using the search functions for all items.

[alexforrester]

@blueglyph - Thanks a lot for for the comments. I'll address them in order.

In regard to the following code:

v?.let { starSign ->
            starSignListener.onSelected(starSign.id)
}

I think your suggestion of the explicit null check could possibly make things clearer, but kotlin's null safe operator ? together with the scope function let is more idiomatic kotlin and is commonplace in most kotlin code so I think it's good to introduce the student/reader to it.

The comment on the syntax of multiple findViewById :

 val starSigns = listOf<View>(
            view.findViewById(R.id.aquarius),
            view.findViewById(R.id.pisces),

...is a valid one. At the time of going to print Kotlin synthetics had just been deprecated and introducing View Binding, the recommended way of accessing Views/Layouts would have been too advanced so early in the title when findViewById had not been properly covered. So for the beginner this was considered preferable.

The counter example could certainly be improved and using 2 mutable counter variables to store as you highlighted is unnecessary and would be better done with a single counter variable as you've shown.

Thanks again for highlighting these issues. Further updates to the book will reflect these suggestions.

Best wishes