Kotlin: Comparing enums

Question

I am currently converting a project from Java to kotlin and re-writing the code in the process to make the most out of kotlin's capabilities. The project contains a Battle System which uses elements to establish inferiority/superiority or neutrality of attacks based on the attacker's and defender's element. The code is much more concise than the java version but I have the feeling there is an even better way to do it, I am just not sure what that way is, so any input would be greatly appreciated, also in regard to improvements in regard to the current code:

internal class Element {

    enum class Type {
        NEUTRAL {
            override fun superiorTo(): List<Type> = emptyList()
            override fun inferiorTo(): List<Type> = emptyList()
        },
        FIRE {
            override fun superiorTo(): List<Type> = listOf(EARTH)
            override fun inferiorTo(): List<Type> = listOf(WATER)
        },
        WATER {
            override fun superiorTo(): List<Type> = listOf(FIRE)
            override fun inferiorTo(): List<Type> = listOf(WIND)
        },
        WIND {
            override fun superiorTo(): List<Type> = listOf(WATER)
            override fun inferiorTo(): List<Type> = listOf(EARTH)
        },
        EARTH {
            override fun superiorTo(): List<Type> = listOf(WIND)
            override fun inferiorTo(): List<Type> = listOf(FIRE)
        };

        private val typeCount = Type.values().size

        abstract fun superiorTo(): List<Type>
        abstract fun inferiorTo(): List<Type>

        fun compare(defender: Type): ComparisonResult {
            val result: ComparisonResult
            if (inferiorTo().contains(defender)) {
                result = ComparisonResult.INFERIOR
            } else if (superiorTo().contains(defender)) {
                result = ComparisonResult.SUPERIOR
            } else {
                result = ComparisonResult.NEUTRAL
            }
            return result
        }
    }

    enum class ComparisonResult {
        NEUTRAL, SUPERIOR, INFERIOR
    }
}

The reason I am going for Lists instead of single types is twofold:

1. account for the option to have inferiorities/superiorities towards multiple elements
2. but mostly to make it easy to take the Neutral element into account and treat it just like all the others

Answer 1

You could use a bidirectional map. In Java syntax this would be:

BidiMap<Type, Type> superior = new BidiMap<>();
superior.put(EARTH, WIND);
superior.put(WIND, WATER);
superior.put(WATER, FIRE);
superior.put(FIRE, EARTH);

That's it. This greatly reduced the redundancy of your code, as well as the verbosity. Since it is a BidiMap, you can query it in both directions.

An alternative approach is to define an array ComparisonResult[typeCount][typeCount​] cmp and initialize that using similar calls as above, again avoiding redundancy. Then your compare method can just return cmp[this.ordinal ()][other.ordinal()].

Answer 2

1. I recommend removing Element.Type.NEUTRAL as "Neutral" is not an Element Type, it is the lack of an element and therefore makes element.type not applicable.

2. With NEUTRAL removed you can greatly simplify your solution:

   internal class Element {
       enum class Type {
           FIRE {
               override val superiorType get() = EARTH
               override val inferiorType get() = WATER
           },
           WATER {
               override val superiorType get() = FIRE
               override val inferiorType get() = WIND
           },
           WIND {
               override val superiorType get() = WATER
               override val inferiorType get() = EARTH
           },
           EARTH {
               override val superiorType get() = WIND
               override val inferiorType get() = FIRE
           };
   
           abstract val superiorType: Type
           abstract val inferiorType: Type
   
           fun compare(other: Type) = when (other) {
               superiorType -> +1
               inferiorType -> -1
               else -> 0
           }
       }
   }
   

   You can still use a ComparisonResult enum if you'd like but this function is very similar to Comparable.compareTo so Int would seem to suffice.

3. Every enum class has a compareTo function implementing Comparable (Enum Classes - Kotlin Programming Language). As such, Element.Type would declare both Element.Type.compare and Element.Type.compareTo which seems confusing to me. I suggest defining superior and inferior check functions instead of Element.Type.compare but this may not be practical depending on how you are using compare:

   infix fun Element.Type.isSuperiorTo(other: Type) = other == superiorType
   infix fun Element.Type.isInferiorTo(other: Type) = other == inferiorType
   

Answer 3

I initially thought it could be made much shorter, but then I started coding it and I realized there are many Kotlin/Java limitations I was unaware of. Namely Kotlin enum can't override compareTo and things can get quite strange due to the ordering of initialization of enum elements. More on that later.

In Kotlin you can use defender in interiorTo() instead of inferiorTo().contains(defender).

Always use the most general type possible. Here that means using Set instead of List since the ordering is irrelevant.

The if-expression in Kotlin is different from the Java if-statement because it returns a value. So you can simplify your code:

return if (defender in inferiorTo()) {
    ComparisonResult.INFERIOR
} else if (defender in superiorTo()) {
    ComparisonResult.SUPERIOR
} else {
    ComparisonResult.NEUTRAL
}

Furthermore it is often cleaner to use Kotlin's when expression rather than a series of if-else:

return when (defender) {
    in inferiorTo() -> ComparisonResult.INFERIOR
    in superiorTo() -> ComparisonResult.SUPERIOR
    else -> ComparisonResult.NEUTRAL
}

I don't like the global structure (Element.Type and Element.ComparisonResult) so much. I think there should only be an Element enum with the five subtypes for clarity. And ComparisonResult should not exist because it is a re-implementation of the Comparable interface. There are already so many APIs that it's not a good idea to introduce new ones. And as a bonus of using existing APIs, there is a lot of code that already exists than can use those, not to mention that people already know how to use APIs such as Comparable.

Unfortunately, as I wrote in the opening paragraph, Kotlin enum implements Comparable as final (not overridable), where the ordering is the order in which the enum elements are defined. However Kotlin also has sealed classes which are nearly identical to enum. I posted some code below implementing your code using a sealed class, and implementing Comparable. I don't like my code so much however and it could probably be improved.

sealed class Element : Comparable<Element> {
    override fun compareTo(other: Element): Int {
        val relations = allRelations[this] ?: throw IllegalStateException("allRelations should cover all values.")
        return when (other) {
            in relations.inferiorTo -> -1
            in relations.superiorTo -> 1
            else -> 0
        }
    }
}
object NEUTRAL: Element()
object FIRE: Element()
object WATER: Element()
object WIND: Element()
object EARTH: Element()

val allRelations: Map<Element, Relations> = mapOf(
        NEUTRAL to Relations(),
        FIRE to Relations(superiorTo = EARTH, inferiorTo = WATER),
        WATER to Relations(superiorTo = FIRE, inferiorTo = WIND),
        WIND to Relations(superiorTo = WATER, inferiorTo = EARTH),
        EARTH to Relations(superiorTo = WIND, inferiorTo = FIRE));

data class Relations(val superiorTo: Set<Element>,
                val inferiorTo: Set<Element>) {
    constructor() : this(setOf(), setOf()) {}
    constructor(superiorTo: Element, inferiorTo: Element) :
            this(superiorTo = setOf(superiorTo), inferiorTo = setOf(inferiorTo))
    init {
        if (!(superiorTo intersect inferiorTo).isEmpty())
            throw IllegalArgumentException("Can't have an Element both superior and inferior.")
    }
}

fun main(args: Array<String>) {
    println(FIRE < WATER)
}

You can see at the very end that I am printing out FIRE < WATER to show that in Kotlin the greater-than and such operators are overloaded to make use of Comparable. The convention for Comparable is that this.compareTo(other) returns a negative number if this is smaller than other.

I tried to make a solution (either using enum or a sealed class) where the superiorTo and inferiorTo would just be specified in the constructors, ie. FIRE(superiorTo = EARTH, inferiorTo = WATER). But I failed since there are some complex issues related to the order in which the objects are created. The worst part is that the code would compile, but then I would get some mysterious runtime error about null references. [EDIT: I updated IntelliJ and it seems this was fixed! For enum there is now a compile time error message and sealed class can now handle out of order references in the constructors. See EDIT below.]

My guest is that the same issue was encountered when developing the original Java code. It struck me as quite odd to create getter methods that create and return a list instead of just having fixed members. It would have been better to just declare those as members in the constructor, which is impossible as described above. I am not a big fan of commenting code everywhere, but I think in this case this should have been documented since it seems wrong if you don't know the issue behind the decision.

In my solution the relationships between the elements are stored in a separate data structure: allRelations. I was forced to do this since I could not declare those relationships in the constructors, ie. FIRE(superiorTo = EARTH, inferiorTo = WATER). But one could argue that it is better to have the elements and their relationships separated. This is the "separation of concerns" principle.

EDIT:

After updating IntelliJ, it seems that Kotlin has evolved in the time it took me to write this post. I can now show an example where the relations are defined directly as constructor members:

sealed class Element(val superiorTo: Set<Element>,
                   val inferiorTo: Set<Element>) : Comparable<Element> {
    constructor(): this(setOf(), setOf()) {}
    constructor(superiorTo: Element, inferiorTo: Element) :
            this(setOf(superiorTo), setOf(inferiorTo))

    override operator fun compareTo(other: Element): Int {
        return when (other) {
            in inferiorTo -> -1
            in superiorTo -> 1
            else -> 0
        }
    }
}

object NEUTRAL: Element()
object FIRE: Element(superiorTo = EARTH, inferiorTo = WATER)
object WATER: Element(superiorTo = FIRE, inferiorTo = WIND)
object WIND: Element(superiorTo = WATER, inferiorTo = EARTH)
object EARTH: Element(superiorTo = WIND, inferiorTo = FIRE)

fun main(args: Array<String>) {
    println(FIRE < WATER)
    println(EARTH < WATER)
    println(EARTH <= WATER)
}

This is much shorter and cleaner. However there is still the issue of deciding if the relationships should be declared separately from the elements ("separation of concerns").