Christmas Tree in Kotlin

Question

Please review my code for printing a Christmas Tree. The idea was to merge some functional programming ideas (immutability, streams) and OOP (every thing is a small object).

I'm looking for a review focused on readability of the code.

Here is a output from the code:

    *    
   ***   
  *****  
 ******* 
*********

And the code:

fun main(a: Array<String>) {
    print(Tree(height = 5))
}

class Tree(val height: Int) {
    override fun toString(): String {
        return (0..height - 1)
                .map { i ->
                    Stage(i, height)
                }
                .joinToString(
                        separator = "\n",
                        postfix = "\n"
                )
    }
}

class Stage(val idx: Int, val height: Int) {
    override fun toString(): String {
        val trunkWidth = TrunkWidth(idx)
        val margin = Margin(MarginWidth(TreeWidth(height), trunkWidth).value())
        return listOf(
                margin,
                Trunk(trunkWidth.value()),
                margin
        ).joinToString(separator = "")
    }

}

class MarginWidth(val tree: TreeWidth, val trunk: TrunkWidth) {
    fun value(): Int {
        return (tree.value() - trunk.value()) / 2
    }
}

class TreeWidth(val height: Int) {
    fun value(): Int {
        return height * 2 - 1;
    }
}

class TrunkWidth(val idx: Int) {
    fun value(): Int {
        return 1 + 2 * idx
    }
}

class Trunk(val size: Int) {
    override fun toString(): String {
        return UniformString(size, "*").toString()
    }
}

class Margin(val size: Int) {
    override fun toString(): String {
        return UniformString(size, " ").toString()
    }
}

class UniformString(val size: Int, val char: String) {
    override fun toString(): String {
        return (0..size - 1)
                .map { char }
                .joinToString(separator = "")
    }
}

Answer 1

1. "args" is more common and descriptive than "a" in a main function signature
2. "\n" is system-dependent; I recommend using System.lineSeparator() instead

3. joinToString can be used with a transform instead of first mapping an Iterable using a transform and then calling joinToString. e.g.:

   (0..height - 1).map { i -> Stage(i, height) }.joinToString()
   

   can be written as

   (0..height - 1).joinToString { i -> "${Stage(i, height)}" }
   

   which will avoid creating an intermediary list (small optimization in this case).

4. Perhaps a minor issue but I personally prefer using "index" instead of "i" or "idx" to avoid any ambiguity. "idx" actually has several meanings.

5. I find buildString used with repeat to often be more readable than Iterable combined with joinToString:

   buildString { repeat(height) { index -> appendln(Stage(index, height)) } }
   

6. I also find buildString to be more suitable than joinToString(separator = ""):

   return buildString {
       append(margin)
       append(Trunk(trunkWidth.value()))
       append(margin)
   }
   

7. If you are going to have classes for computed numbers than I find toInt (like AtomicInteger.toInt) much more readable than value for MarginWidth, TreeWidth, etc.

8. Naming an instance of TreeWidth as simply "tree" is rather confusing, I recommend using type names (at least the end of them) at the end of instance names (e.g. "stageTreeWidth", "stageWidth", "treeWidth", or simply "width").

9. Kotlin has a neat String.repeat function to simplify building strings with a repeated char sequence. e.g.:

   "*".repeat(size)
   

10. Technically a "trunk" does not include the branches, etc. so "TrunkWidth" and "Trunk" are kind of confusing names for this. You might consider "LayerWidth" and "Layer".

11. I see very little point to all these classes as they do not serve much purpose. A class with a single method to define its single behavior might as well be a method. If Stage, MarginWidth, Margin, etc. will be used by methods other than Tree.toString then it might make sense to have these as classes. If they are simply to build a string representation then I would drop them in favor of inline calculations (OOP does not mean that "every thing is a small object"; e.g. "height" and "width" may be attributes of objects but are not necessarily objects themselves).

    This entire collection of classes can be reduced to the following (assuming no other code is using Stage, Margin, etc.):

    class Tree(val height: Int) {
        override fun toString(): String {
            return buildString {
                val treeWidth = height * 2 - 1
                repeat(height) { index ->
                    val layerWidth = 1 + 2 * index
                    val marginWidth = (treeWidth - layerWidth) / 2
                    val marginString = " ".repeat(marginWidth)
                    val layerString = "*".repeat(layerWidth)
                    append(marginString)
                    append(layerString)
                    append(marginString)
                    appendln()
                }
            }
        }
    }
    

    If Tree.height is never used then even Tree itself can be eliminated and the tree string building logic can be placed in a single function:

    fun buildTreeString(height: Int) = buildString {
        repeat(height) { layerIndex ->
            val layerWidth = 1 + 2 * layerIndex
            val marginWidth = height - layerIndex - 1
            repeat(marginWidth) { append(' ') }
            repeat(layerWidth) { append('*') }
            repeat(marginWidth) { append(' ') }
            appendln()
        }
    }
    

    And you can even pre-compute the required capacity to avoid unnecessary array copying while building the string:

    fun buildTreeString(height: Int): String {
        return buildString(height * (2 * height - 1 + System.lineSeparator().length)) {
            repeat(height) { layerIndex ->
                val layerWidth = 1 + 2 * layerIndex
                val marginWidth = height - layerIndex - 1
                repeat(marginWidth) { append(' ') }
                repeat(layerWidth) { append('*') }
                repeat(marginWidth) { append(' ') }
                appendln()
            }
        }
    }
    
    inline fun buildString(capacity: Int, builderAction: StringBuilder.() -> Unit)
            = StringBuilder(capacity).apply(builderAction).toString()
    

    If however you actually need/want classes for tree components then I recommend making those components (or collections of those components) members of a Tree class (e.g. a Tree has layers: List<Tree.Layer>).