6
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Jun 5 '17 at 21:17
3
\$\begingroup\$

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()].

\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestions! The BidiMap only works for the cases where superiority and inferiority is symmetric right? For my current use-case that actually suffices, so I'll give it a shot and mark this as accepted if it works out well. I prefer the BidiMap approach over the array, but I guess the array has better performance? Thank you! \$\endgroup\$ – AgentKnopf Jun 3 '17 at 9:56
  • \$\begingroup\$ Yes, the symmetric property is important here. And, thinking about it again, a simple BidiMap won't work for Rock Paper Scissors Lizard Spock; you would need a BidiMultimap there. Or the array. In terms of performance, the array is probably best since it needs exactly 3 memory accesses. For the maps, it depends on the implementation. \$\endgroup\$ – Roland Illig Jun 3 '17 at 10:30
  • \$\begingroup\$ I decided on the 2-dimensional array. For the reasons you mentioned and to avoid having to add a 3rd party library just for that BidiMap. Since kotlin 2-dimen arrays are too verbose, I switched to Java for this implementation. Thank you! \$\endgroup\$ – AgentKnopf Jun 6 '17 at 8:11
1
\$\begingroup\$
  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
    
\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestion. Unfortunately Neutral is an element - imagine a pokemon-style battle game. It pretty much correpsonds to "Normal" types - so maybe "Normal" makes for a better name than "Neutral". I agree, that the ComparisonResult is probably something I could get rid of. I find the approach you took really intriguing, but would it hold up if there are multiple types a given element is inferior/superior towards? \$\endgroup\$ – AgentKnopf Jun 11 '17 at 18:50
  • 1
    \$\begingroup\$ I see, then a NORMAL enum value would make sense and yes, if you wanted a list of superior types/values then your current seems fine (with a function returning a list of values). You could still do isSuperiorTo using other in superiorTypes() or something similar. \$\endgroup\$ – mfulton26 Jun 12 '17 at 0:37
1
\$\begingroup\$

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").

\$\endgroup\$
  • \$\begingroup\$ Hey, first of all: Thank you SO much for investing this huge amount of time into your answer!! I really liked following your train of thought on the different solutions! And thank you for taking the time to comment on a few things i should have avoided (comparable etc) - that's a huge help! I'll take some time to check your proposals more in-depth and see what works best for my scenario - thanks again! \$\endgroup\$ – AgentKnopf Aug 1 '17 at 18:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.