Java and Kotlin antipattern evolution.md

Singleton

Roll-your-own lazy singleton

public final class Single {
    private static Single INSTANCE;
    private Single() {}
    
    public static Single getInstance() {
        return INSTANCE != null
                ? INSTANCE : (INSTANCE = new Single());
    }
}

Roll-your-own DCL singleton

public final class Single {
    private static final Object LOCK = new Object();
    private static volatile Single INSTANCE;
    private Single() {}
    
    public static Single getInstance() {
        Single inst = INSTANCE;
        if (inst != null) return inst;
        synchronized(LOCK) {
            if ((inst = INSTANCE) != null) return inst;
            return INSTANCE = new Single();
        }
    }
}

Enum as singleton

public enum Singleton { INSTANCE; }

Healthy person's singleton:

public final class Single {
    private static final Single INSTANCE = new Single();
    private Single() {}

    public static Single getInstance() {
        return INSTANCE;
    }
}

object Single

A singleton with a parameter:

public final class Single {
    private static Single INSTANCE;
    public static Single getInstance(Context context) {
        return INSTANCE != null
                ? INSTANCE : (INSTANCE = new Single(context));
    }
    private final Context context;
    private Single(Context context) {
        this.context = context;
    }
}

class Single
private constructor(private val context: Context) {
    companion object {
        private var INSTANCE: Single? = null
        fun getInstance(context: Context): Single {
            INSTANCE?.let { return it }
            return Single(context).also { INSTANCE = it }
        }
    }
}

Healthy person's class with a parameter:

public final class NotSingle {
    private final Context context;
    public NotSingle(Context context) {
        this.context = context;
    }
}

class NotSingle(private val context: Context)

Nullability hell

var callback: Callback? = null
override fun onAttach(context: Context?) { // 1
    this.callback = context as? Callback // 2
}
...
override fun onClick(v: View?) { // 3
    (v?.tag // 4
            as? SomeObj)?.let { obj -> //5
        callback?.objClicked(obj) // 6
    }
}

Strict way:

lateinit var callback: Callback
fun onAttach(context: Context) {
    this.callback = context as Callback
}
...
override fun onClick(v: View) {
    callback.objClicked(v.tag as SomeObj)
}

Explicit strict way:

fun onAttach(context: Context) {
    this.callback = context as? Callback
            ?: throw ClassCastException("host context must implement my Callback")
}
...
override fun onClick(v: View) {
    callback.objClicked(
            v.tag as? SomeObj
                    ?: throw AssertionError("view $v expected to have a tag of type SomeObj, got ${v.tag}")
    )
}

Exception to this rule is where everything really can be nullable:

private val PsiReference.outerMethodName: String? get() {
    val el = element

    // Java
    PsiTreeUtil.getParentOfType(el, PsiMethodCallExpression::class.java)?.methodExpression
        ?.takeIf { it.qualifierExpression === this }
        ?.let { (it.reference?.resolve() as PsiMethod?)?.name }
        ?.let { return it }

    // Kotlin
    PsiTreeUtil.getParentOfType(el, KtDotQualifiedExpression::class.java)
        ?.takeIf { it.receiverExpression.references.any { it.element == el } }
        ?.let { (it.selectorExpression as? KtCallExpression)?.calleeExpression?.references }
        ?.forEach { (it.resolve() as? PsiMethod)?.name?.let { return it } }

    return null
}

Type checking

if (iterable instanceof Collection) { ...

Even Java stdlib:

public static <T> List<T> unmodifiableList(List<? extends T> list) {
    return (list instanceof RandomAccess ?
            new UnmodifiableRandomAccessList<>(list) :
            new UnmodifiableList<>(list));
}

Perfectly valid code and bad code which breaks it. Oops! Even equals() is broken thanks to typecasting. equals(Object) is wrong; interface Comparable<T> { int compareTo(T) } is great and right.

Kotlin version with 'safe cast':

val Iterable<*>.size: Int
    get() = (this as? Collection<*>)?.size ?: iterator().size

val Iterator<*>.size: Int
    get() {
        var count = 0
        while (hasNext()) {
            next(); count++
        }
        return count
    }

but it's not safer!

data classes with no reason
```
data class User(val name: String, val age: Int)
```
The compiler automatically derives the following members from all properties declared in the primary constructor:
- equals()/hashCode() pair;
- toString() of the form "User(name=John, age=42)";
- componentN() functions corresponding to the properties in their order of declaration;
- copy() function (see below).
Destructuring of a non-tuple
```
val (name, age) = user
```

Labeled return

args.forEachIndexed { idx, arg ->
    visitParameter(
        params.getOrNull(idx)?.second ?: return@forEachIndexed,
        when (val it = arg.type) {
            PsiType.NULL -> null
            is PsiClassType -> it.resolve() ?: return@forEachIndexed
            is PsiPrimitiveType -> it.getBoxedType(arg)?.resolve() ?: return@forEachIndexed
            else -> return@forEachIndexed
        },
        arg.endOffset,
        collector
    )
}

Healthy person's return from anonymous:

args.forEachIndexed(fun(idx: Int, arg: PsiExpression) {
    visitParameter(
        params.getOrNull(idx)?.second ?: return,
        when (val it = arg.type) {
            PsiType.NULL -> null
            is PsiClassType -> it.resolve() ?: return
            is PsiPrimitiveType -> it.getBoxedType(arg)?.resolve() ?: return
            else -> return
        },
        arg.endOffset,
        collector
    )
})

open classes and functions

Kotlin defaults are better! 'allopen' is a workaround.

default method emulation — OK

/**
 * Simple adapter class for [TextWatcher].
 */
open class SimpleTextWatcher : TextWatcher {
    /** No-op. */ override fun afterTextChanged(s: Editable) {}
    /** No-op. */ override fun beforeTextChanged(s: CharSequence, start: Int, count: Int, after: Int) {}
    /** No-op. */ override fun onTextChanged(s: CharSequence, start: Int, before: Int, count: Int) {}
}

'template method' pattern — OK
sharing logic within private scope, class count/size economy — OK

DTO/VO with default values, default constructor, and/or mutable fields

class User(
    var name: String? = null,
    var age: Int = 0,
)

Single operation on a Sequence

array.map(Element::something)
vs.
array.asSequence().map(Element::something).toList()

Multiple operations on a collection

array
    .filter { it.isCool }
    .map(Element::something)
    .sortedBy(Element::size)
vs.
array
    .asSequence()
    .filter(Element::isCool)
    .map(Element::something)
    .sortedBy(Element::size)
    .toList()

Oh, we have map+filter in one run and in-place sorting!

array
    .mapNotNull { if (it.isCool) it.something else null }
    .sortBy(Element::size)

interface Constants

public interface Constants {
    int A = 1;
}
public class Whatever implements Constants {
    ...
}

const val A = 1;

Nope! Constants are implementation details. They should be private or passed via constructor!

Enum as a stupid constant

Bad:

enum Role {
    User, Admin
}
// ...
@DrawableRes int iconOf(Role role) {
    switch (role) {
        case User: return R.drawable.icon_role;
        case Admin: return R.drawable.icon_admin;
        default: throw new AssertionError();
    }
}

Better:

enum Role {
    User(R.drawable.icon_role),
    Admin(R.drawable.icon_admin),
    ;
 
    @DrawableRes public final int icon;

    Role(@DrawableRes int icon) {
        this.icon = icon;
    }
}

OK for Kotlin, thanks to exhaustive when:

enum class Role {
    User, Admin
}
// ...
@DrawableRes fun iconOf(role: Role): Int = when (role) {
    Role.User -> R.drawable.icon_role
    Role.Admin -> R.drawable.icon_admin
}

Reflect

Given Java/Kotlin enum serialized as enum:

enum class Role { USER, ADMIN }

...evolves to...

sealed class Role {
    object User : Role()
    object Admin : Role()
}

um, where are SCREAMING_CASE names? will it break serialization? Healthy enum representation for serialization:

mapOf(
    "USER" to Role.User,
    "ADMIN" to Role.Admin,
)

listOf(Role.User, Role.Admin), Role::asString

@JsonAdapter(DateAdapter.class)
Date date1;

@JsonAdapter(DateAdapter.class)
Date date2;

...and after some time we have two date formats...

class DateAdapter1 : TypeAdapter by DateAdapter(SimpleDateFormat("format #1"))
class DateAdapter2 : TypeAdapter by DateAdapter(SimpleDateFormat("format #2"))

Healthy code never mandate components to have no-arg constructor!

Implicit registration, e. g. Map<Class<T>, T>

Java:
- what about generic types?
- runtime fail if required instance is not registered
- nothing happens if useless instance is registered
- different instances of the same type for different cases?
Kotlin Map<KType, instance>:
```
-what about generic types?
+wildcards and projections?
```
In the wild: Gson TypeAdapters, Jackson modules, DI containers, classpath scanning.
Annotation processing

Code generation is same reflection, but at compile-time. This implies:

− it does not work with separate compilation

+ it is more type-safe

− larger bytecode

+ faster bytecode

Property delegation, an interesting but risky case of compile-time reflection

class User(map: Map<String, Any>) {
    val name: String by map
    val age: String by map
}

Symbol name becomes a String! Refactoring breaks serialization ABI.

Providing serialized name explicitly, just a fantasy:

class User(map: Map<String, Any>) {
    val "name": String by map
    val "age": String by map
}
class User(map: Map<String, Any>) {
    val firstName: String by map as "name"
    val yearsAlive: String by map as "age"
}

Object identity
```
if (a == b) // Java
```
```
if (a === b) // Kotlin
```
Can be useful as an optimization, but could also indicate a design failure.
DSLs

DSLs are useful for streaming (kotlinx.html) or wrapping legacy APIs (like Android Views: Anko, Splitties, etc). But if you need to create an object graph, just do it: cann constructors and pass objects there, without any wrappers, like Flutter UI framework does.

Mappers

Smth smth = new Smth();
smth.setA(someDto.getA());
smth.setB(someDto.getX().getB());
smth.setC(someDto.getX().getC());

Smth(
    a = someDto.a,
    b = someDto.x.b,
    c = someDto.x.c
)

Solution: fix deserializer, SQL query, or whatever.

Convenience methods and overloads

Imagine a TextView which has setText(CharSequence) method. That's okay.

Now we add setText(@StringRes int resId) version which just does setText(res.getText(resId)). Why this is bad?
- SRP is broken. Now you're not only a TextView, you're also an, um, ResourcesAccessor.
- We're just on our way to add more convenience methods. What about setText(@StringRes int resId, Object... formatArgs) and setQuantity(@PluralsRes int id, int quantity, Object... formatArgs)?
- You use a TextView somewhere, maybe in a dialog builder. Should you support all these overloads in your interface, too? Or maybe find which one is primary and which are convenience?
Another bloated examples from Android SDK:
- View.setBackground: setBackgroundColor, setBackgroundResource
- ImageView.setImageDrawable: setImageResource, setImageURI, setImageIcon, setImageBitmap
So, where are setBackgroundBitmap or setImageColor?

Kotlin: it's okay to add convenience extension functions:
- they don't bloat the class
- more overloads can be added without editing class sources
- in IDE, extensions can be easily distinguished from member functions by their appearance

Miha-x64/Java and Kotlin antipattern evolution.md

Singleton