Improving code quality - Session 3: Strategy without a strategy

Hello, I'm Ishikawa, and I develop the mobile clients for the LINE messaging app.

This article is the third installment of our weekly series Weekly Report. For more information about the Weekly Report, please see the first article.

Strategy without a strategy

The Loggable interface below holds information for log output.

interface Loggable {
    val logType: LogType
    val logLevel: LogLevel
    val logDescription: String
    val timestamp: Long
    val codeLocation: StackTraceElement
    ...
)

Assume we needed an enumeration type called LogAttribute to decide which properties to output as logs. The function createLogMessage decides which properties to use for constructing the log message based on the given LogAttribute.

enum class LogAttribute { LOG_TYPE, LOG_LEVEL, LOG_DESCRIPTION, ...}

fun createLogMessage(loggable: Loggable, attributesToLog: Set<LogAttribute>): String {
  ...
}

For example, if setOf(LOG_LEVEL, LOG_TYPE, LOG_DESCRIPTION) is given as attributesToLog, we expect the log message to contain the log level, type, and description, such as "FATAL, Crash, something wrong with some parameter".

Furthermore, let's assume the order of LogAttribute is statically determined and doesn't need to be changed. For instance, there might be a rule that says "log levels should be positioned at the beginning of the message". To achieve this rule, developers might define a list (ORDERED_ATTRIBUTES_TO_LOG) to determine the order and use it to construct the log message.

// This order might be different from `ordinal` of `LogAttribute`.
val ORDERED_ATTRIBUTES_TO_LOG: List<LogAttribute> = listOf(
  LOG_LEVEL,
  LOG_TYPE,
  LOG_DESCRIPTION,
  ...
)

fun createLogMessage(loggable: Loggable, attributesToLog: Set<LogAttribute>): String =
    ORDERED_ATTRIBUTES_TO_LOG.asSequence()
        .filter(attributesToLog::contains)
        .map { attribute ->
            when (attribute) {
                LogAttribute.LOG_LEVEL -> getLogLevelText(loggable)
                LogAttribute.LOG_TYPE -> getLogTypeText(loggable)
                ...
            }
        }.joinToString()

Is there a problem with this code?

Repeated "advanced flexibility"

This code has a structure where there are branches inside a loop, and each branch is used at most once in the loop.

This leads to two problems:

• Since the branches are directly written inside the loop, you need to understand each branch to grasp the function's flow.
• It's hard to match the "type" order with the branches.

To solve these problems, here are four options. Each has its pros and cons, so you need to choose appropriately.

Option 1: Remove the loop

If the "collection indicating types" is small enough, you might directly branch with if for each element.

val message = StringBuilder()
if (attributesToLog.contains(LogAttribute.LOG_LEVEL)) {
    message.append(getLogLevelText(loggable))
}
if (attributesToLog.contains(LogAttribute.LOG_TYPE)) {
    message.append(getLogTypeText(loggable))
}
...

Creating a helper function like getAttributeTextOrEmpty below might improve clarity.

val message =
    loggable.getAttributeTextOrEmpty(attributesToLog, LogAttribute.LOG_LEVEL, ::getLogLevelText) +
    loggable.getAttributeTextOrEmpty(attributesToLog, LogAttribute.LOG_TYPE, ::getLogTypeText) +
    ...

private fun Loggable.getAttributeTextOrEmpty(
    attributesToLog: Set<LogAttribute>,
    targetAttribute: LogAttribute,
    attributeTextCreator: (Loggable) -> String
): String = if (targetAttribute in attributesToLog) attributeTextCreator(this) else ""

However, this option has downsides. The first is that it's hard to write unit tests to check if all types are covered. If a new type is added, there's little warning about implementation omissions. Secondly, you can't use collection utility functions like joinToString. (In the code above, I omitted the code to connect with ", ". Implementing it would make the code more complex.)

Option 2: Extract branches

Simply extracting conditional branches as helper functions is another method.

fun createLogMessage(loggable: Loggable, attributesToLog: Set<LogAttribute>): String =
    ORDERED_ATTRIBUTES_TO_LOG.asSequence()
        .filter(attributesToLog::contains)
        .map { attribute -> attribute.getLogText(loggable) }
        .joinToString()

private fun LogAttribute.getLogText(loggable: Loggable): String = when(this) {
    LogAttribute.LOG_TYPE -> ...
    ...
}    

However, while this method somewhat improves the readability of the flow, it doesn't improve the matching between ORDERED_ATTRIBUTES_TO_LOG and getLogText. Also, using else (or default) in branches might make it impossible to ensure exhaustiveness, so it's safer not to use else.

Option 3: Embed logic in types

By applying the strategy pattern or a similar structure, you can embed a specific logic in each type.

enum class LogAttribute {
  LOG_TYPE {
    override fun getLogText(loggable: Loggable) = ...
  },
  LOG_LEVEL {
    override fun getLogText(loggable: Loggable) = ...
  },
  ...;

  abstract fun getLogText(loggable: Loggable)
}

Using this, getLogText becomes as follows:

fun createLogMessage(loggable: Loggable, attributesToLog: Set<LogAttribute>): String =
    ORDERED_ATTRIBUTES_TO_LOG.asSequence()
        .filter(attributesToLog::contains)
        .map { attribute -> attribute.getLogText(loggable) }
        .joinToString()

This makes the function flow easier to understand since you can't see branches from createLogMessage. One advantage of this option is easier guarantee of exhaustiveness. If there's an implementation omission, it results in a compile error, and it's easier to write unit tests to ensure ORDERED_ATTRIBUTES_TO_LOG contains all elements.

Moreover, you might embed a property like val logOrder: Int in each type to create ORDERED_ATTRIBUTES_TO_LOG.

val ORDERED_ATTRIBUTES_TO_LOG: List<LogAttribute> =
    LogAttribute.values().sortedBy(LogAttribute::logOrder)

However, this option is only viable if the "type" is not used elsewhere. Embedding specific functionality's logic or values into a widely used type can complicate dependencies.

Option 4: Create a tuple to explicitly relate

To explicitly show the relationship between type, order, and logic, you can create a tuple like the AttributeTextModel below.

class AttributeTextModel(val attributeType: LogAttribute, val textCreator: (Loggable) -> String)

createLogMessage would then be:

val ORDERED_ATTRIBUTES_TO_LOG: List<AttributeTextModel> = listOf(
    AttributeTextModel(LogAttribute.LOG_LEVEL, ::getLogLevelText),
    AttributeTextModel(LogAttribute.LOG_TYPE, ::getLogTypeText),
    ...
)

fun createLogMessage(loggable: Loggable, attributesToLog: Set<LogAttribute>): String =
    ORDERED_ATTRIBUTES_TO_LOG.asSequence()
        .filter { attributesToLog.contains(it.attributeType) }
        .map { it.textCreator(loggable) }
        .joinToString()

This ensures that "if a type's order is defined, the corresponding logic attributesToLog is also implemented". Unlike Option 3, this allows you to attach specific logic to widely used types. However, this code does not guarantee "for every type, there's corresponding logic", but you can cover this with unit tests. The test would verify that "for each LogAttribute, there's a corresponding AttributeTextModel within ORDERED_ATTRIBUTES_TO_LOG".

---

In summary: When there are significant conditional branches inside a loop, consider restructuring to make the relationship between branches and logic clearer.

Keywords: loop, conditional branch, strategy pattern