Skip to content

6.) Anti Patterns

Gabor Varadi edited this page Feb 15, 2021 · 16 revisions

Using !! where it's not necessary

var realm: Realm? = null
var results: RealmResults<T>? = null

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    realm = Realm.getDefaultInstance()

    results = realm!!.where<T>().findAll()
    results!!.addChangeListener { /* ... */ }
    realm!!.addChangeListener { /*...*/ }
}

override fun onDestroy() {
    super.onDestroy()
    realm!!.removeAllChangeListeners()
    realm!!.close()
    realm = null
}

could be

var realm: Realm? = null
lateinit var results: RealmResults<T>

override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    val realm = Realm.getDefaultInstance()
    this.realm = realm

    results = realm.where<T>().findAll()
    results.addChangeListener { /* ... */ }
    realm.addChangeListener { /*...*/ }
}

override fun onDestroy() {
    super.onDestroy()
    realm?.removeAllChangeListeners()
    realm?.close()
    realm = null
}

Using it in multi-line expression (1.)

.map {
    Item(
        id = it.id,
        title = it.title,
        timestamp = it.createdAt,
        author = it.user.name
    )
}

should be

.map { photo ->
    Item(
        id = photo.id,
        title = photo.title,
        timestamp = photo.createdAt,
        author = photo.user.name
    )
}

Using it in multi-line expression (2.)

activity?.let {
    sharedViewModel = ViewModelProvider(it).get(SharedViewModel::class.java)
}

Prefer to name variables if the lambda is not in the same line.

activity?.let { activity ->
    val viewModel = ViewModelProvider(activity).get(MyViewModel::class.java)
}

Or another option:

val activity = activity
if(activity != null) { // <-- this enables safe-casting to non-null type
    val viewModel = ViewModelProvider(activity).get(MyViewModel::class.java) 
    ...
}

Also note that the ViewModelProviders.of(activity) could be moved into an extension function, and the (MyViewModel::class.java) could be replaced with <MyViewModel>() using inline fun <reified T: ViewModel>.

Nested its

activity?.let { // it
    val viewModel = ViewModelProviders.of(it).get(MyViewModel::class.java)

    viewModel.inputNumber.observe(this, Observer { // it
        it?.let { // it
            ...
        }
    })
}

should be

val activity = activity
if(activity != null) {
    val viewModel = ViewModelProviders.of(activity).get(MyViewModel::class.java)

    viewModel.inputNumber.observe(viewLifecycleOwner, Observer {
        val number = it ?: return@Observer
        ...
    })
}

Unnecessary chaining of safe-call operator

activity?.supportFragmentManager
        ?.beginTransaction()
        ?.setCustomAnimations(...)
        ?.replace(...)
        ?.addToBackStack(...)
        ?.commit()

should be

activity?.let { activity ->
    activity.supportFragmentManager
            .beginTransaction(...)
            .setCustomAnimations(...)
            .replace(...)
            .addToBackStack(...)
            .commit()
}

Using var instead of val + assignment

var input = 0
if (text.toString().isNotEmpty()) {
    input = text.toString().toInt()
}

Could be

val str = text.toString()
val input = when {
    str.isNotEmpty() -> str.toInt()
    else -> 0
}

Assigning to the same value multiple times on different branches (even though there is nothing else in the condition)

if (p1 == 0) {
    appBar.stateListAnimator =  AnimatorInflater.loadStateListAnimator(context, R.animator.appbar_elevation_disable)
} else {
    appBar.stateListAnimator =  AnimatorInflater.loadStateListAnimator(context, R.animator.appbar_elevation_enable)
}

should be

appBar.stateListAnimator =  AnimatorInflater.loadStateListAnimator(context, when { 
    p1 == 0 -> R.animator.appbar_elevation_disable
    else -> R.animator.appbar_elevation_enable
})

Separation of field declaration and constructor (where the two could be merged)

class Device_info {
    var id : Int = 0
    var name : String = ""

    constructor(id : Int, name: String){
        this.id = id
        this.name = name
    }
}

should be

class DeviceInfo(
    var id : Int = 0,
    var name : String = ""
) 

Using + to concat strings instead of """

val createTable = "CREATE TABLE " + TABLE_NAME + "(" + RAW_ID +
       " INTEGER PRIMARY KEY, " + COLUMN_NAME + " TEXT, " +
         COLUMN_ID + "INTEGER)"

should be

val createTable = """CREATE TABLE $TABLE_NAME(
                      $RAW_ID INTEGER PRIMARY KEY, 
                      $COLUMN_NAME TEXT, 
                      $COLUMN_ID INTEGER)"""

Using var instead of val where setter is not required

class DataBaseHandler(var context: Context) : SQLiteOpenHelper(context, DATABASE_NAME, null, 1) {

could be

class DatabaseHandler(val context: Context) : SQLiteOpenHelper(context, DATABASE_NAME, null, 1) {

Using nullable platform type where the object is actually not nullable

override fun onCreate(db: SQLiteDatabase?) {
    db?.execSQL(createTable)
}

could be

override fun onCreate(db: SQLiteDatabase) {
    db.execSQL(createTable)
}

Using "m" prefix for field variables

var mPosition:Int=adapterPosition
var item:TestCart=mList[mPosition]

when(v!!.id)
{
    deleteButton.id ->
    {
        deleteItem(item.itemId!!)
        mList.removeAt(adapterPosition)

could be

// assume I'm using `.onClick {` instead
val item = itemList[adapterPosition]
deleteItem(item.itemId)
itemList.removeAt(adapterPosition)

Declaring field variables in constructor on the right side of the screen

class DeviceInfo(var id : Int = 0,
                 var name : String = ""): Entity()

could be

class DeviceInfo(
    var id : Int = 0,
    var name : String = ""
): Entity()

Not using scoping functions where they actually make the code simpler

var values = ContentValues()
values.put("Title", "Some title")
values.put("Content", "And some content")

could be

val values = ContentValues().apply {
    put("Title", "Some title")
    put("Content", "And some content")
}

Exposing statics to Java through companions

SomeClass someClass = MyObject.Companion.INSTANCE.getSomeClass();

should be

SomeClass someClass = MyObject.SOME_CLASS;

with

class MyObject {
    companion object {
        @JvmField // <-- in some cases, @JvmStatic 
        val SOME_CLASS = SomeClass()
    }
}