Simple Kotlin Bingo Class

Question

This is a simple class in Kotlin that handles three basic Bingo-related functionalities: Specifying the numbers, generating a card, and calling one random element. Additionally, it should support an empty constructor, and support Discord Emoji output, in which there should be no spaces between any elements. I personally think drivers that use this class should do their own error handling.

I'd like suggestions on how to make this more in line with Kotlin standards, e.g. idioms, code style, whether or not I should delegate error handling to drivers, alternate/more readable approaches to certain subtasks...

Edit: I have changed the code to add an additonal constructor that specifies the delimiting char between elements.

Edit on constructor: The constructor splits the input string along the spl val and puts the first element into delim. For the rest, if the first character isn't : (meaning the input isn't Discord Emoji), the elements are sorted by length alone before being padded with spaces so 1. all elements are of the same length 2. all elements have a trailing space. Then, the header is initialized with the first 5 elements of the Column enum using the same padding conditions. For Discord Emoji input, the header is initialized similarly but with regional indicators.

import kotlin.random.Random
class Bingo() {
 var num = (1..75).map { if (it < 10) " $it " else "$it " }
 var cur = num.associateBy({ num.indexOf(it) }, { it }).toMutableMap()
 var delim = " ∅ "
 var max = 2
 var header = "B I N G O"
 enum class Column {
 B,
 I,
 N,
 G,
 O,
 Empty
 }
 /**
 * It is expected that the number of elements ≥ 25 and is divisible by 5
 */
 constructor(nums: String, spl: Char) : this() {
 val rem = nums.substringAfter(spl).split(spl).toMutableList()
 while (rem.contains("")) rem.remove("")
 while (rem.size % 5 != 0) rem.remove(rem.random())
 if (rem.size < 25) return
 num = rem; delim = nums.substringBefore(spl)
 if (num[0][0] != ':') {
 num = num.sortedBy { it.length }.toMutableList()
 max = num.last().length
 num = num.map { " ".repeat(max - it.length) + it + " " }
 delim = " ".repeat(max - delim.length) + delim + " "
 header = ""
 for (i in (0..4)) header += "" + Column.values()[i] + " ".repeat(max)
 } else {
 header = ":regional_indicator_" + Column.values()[0].toString().lowercase()
 //Reg. indicators often turn into flags without spaces
 for (i in (1..4)) header += " :regional_indicator_" + Column.values()[i].toString().lowercase()
 }
 cur = num.associateBy({ num.indexOf(it) }, { it }).toMutableMap()
 }
 constructor(nums: String): this(nums, '@')
 fun card(): Array<Array<String>> {
 val ret = Array(5) { Array(5) { "" } }
 var index: Int
 val done = mutableListOf<Int>()
 for (i in (1..5))
 for (j in (1..5)) {
 if (i == j && j == 3) {
 ret[2][2] = delim
 continue
 }
 do {
 index = Random.nextInt(num.size / 5 * (j - 1), num.size / 5 * j)
 } while (done.contains(index))
 ret[i - 1][j - 1] = num[index]
 done.add(index)
 }
 return ret
 }
 fun call(): Pair<Column, String> {
 if (cur.isEmpty()) return Pair(Column.Empty, "")
 var index: Int
 do index = Random.nextInt(num.size) while (cur[index] == null)
 val list = cur[index]!!.trim()
 cur.remove(index)
 return Pair(Column.values()[index / (num.size / 5)], list)
 }
}
```

Answer 1

Let's start with the basics:

Use meaningful names. Names such as num, cur, spl and rem are short but they don't make the code more readable. The name list is especially confusing since its type is a String.

Never use ; to separate statements on one line as it makes the code cluttered.

Don't use mutable names (variables that can be reassigned) without a good reason, as this practice is error-prone and mutability-related errors are often very hard to debug. Always use read-only names (val) where you can. In this particular program every single var can be replaced with a val, consider doing it as an exercise.

---

Let's go back to the top.

Kotlin supports primary constructors. Always use this feature to its fullest by making the largest constructor primary. Any other constructors will call the primary (or each other) with some defaults:

class A(
 val foo: String,
 val bar: Int,
) {
 constructor() : this(
 DEFAULT_FOO,
 DEFAULT_BAR,
 )
}

The second constructor takes 20 lines to parse some nums string. This code is very hard to comprehend since there is no format for this string specified anywhere. There should be a constructor with everything the class really needs (num, cur, etc..) and another one that takes the string you want to parse.

---

There are default parameters in kotlin: instead of

constructor(nums: String, spl: Char) : ...
constructor(nums: String): this(nums, '@')

write

constructor(nums: String, spl: Char = '@') : ...

---

fun card(): Array<Array<String>>

Avoid using arrays, they have a lot of disadvantages compared to the modern List interface (e.g. mutability and invariance).

---

Try using kotlin standard library more, it makes the code more readable and concise when used wisely. E.g. this

fun card(): Array<Array<String>> {
 val ret = Array(5) { Array(5) { "" } }
 var index: Int
 val done = mutableListOf<Int>()
 for (i in (1..5))
 for (j in (1..5)) {
 if (i == j && j == 3) {
 ret[2][2] = delim
 continue
 }
 do {
 index = Random.nextInt(num.size / 5 * (j - 1), num.size / 5 * j)
 } while (done.contains(index))
 ret[i - 1][j - 1] = num[index]
 done.add(index)
 }
 return ret
}

can be rewritten as this:

fun test(): List<List<String>> = num
 .shuffled()
 .slice(0 until 25)
 .toMutableList()
 .also { it[12] = delim }
 .chunked(5)

• Aaron Liu: Thanks, but I didn't set variables to private or immutable because I want the driver to be able to access and mutate them. I'm not sure how short variable assignments on the same line is cluttered. I explained the constructor in an edit, and as Kotlin constructors need to build on the default I think the one that builds upon the default should be secondary. I appreciate all these other language features you said though.
• QuasiStellar: @AaronLiu what's a driver? I suppose you mean a client. Every interaction with your code should go through an interface (in this case it is represented by public method), this way you make sure it is not misused. Making implementation details public and mutable does no good, that's the basics of encapsulation. As for the other comment, there is no need in explanations when the code itself is not readable. If you find yourself writing comments explaining what the code does instead of why these things are done, you're better off rewriting the code.
• Aaron Liu: Yes, I mean a client, thanks. I don't believe in Java's strict encapsulation, and Kotlin seems to believe that as well with the conversion of many functions to fields. These (save for cur, that one I can private and val) aren't just implementation details; I consider them a part of the "output", partially since they cannot cause errors. I don't see how the constructor isn't readable, it mainly uses Kotlin inbuilt functions and often reuses code. My brother can also understand it.
• Aaron Liu: Actually I don't think I'm going to val cur as I'll be adding a method to reset it. In fact, I don't think any of these top vars can be val'd even if I want to as vals can't have lateinit and I have two different constructors with different values that'll need to be assigned to these vars.
• QuasiStellar: @AaronLiu "I don't see how the constructor isn't readable" You came up with these variable names so they make sense to you, but not necessarily to others. "Kotlin seems to believe that as well with the conversion of many functions to fields" This has nothing to do with encapsulation. Consider reading more on the topic (for example here: stackoverflow.com/a/18301036/16763958)