Hello, I'm Munetoshi Ishikawa, a mobile client developer for the LINE messaging app.
This article is the latest installment of our weekly series "Improving code quality". For more information about the Weekly Report, please see the first article.
Wild arguments
Suppose the data for a "shape" is represented as a string separated by spaces. Here is an example of such a string. The first value separated by spaces is called a "tag" indicating the type of shape, and the subsequent values are attributes (such as coordinates) for each shape.
circle 10 20 30
rectangle 10 20 30 40
line 10 20 30 40Let's implement code to interpret this string. The data for each line is converted into a predefined data model as follows.
class Circle(centerX: Int, centerY: Int, radius: Int): Shape
class Rectangle(left: Int, top: Int, right: Int, bottom: Int): Shape
class Line(startX: Int, startY: Int, endX: Int, endY: Int): ShapeThe format of the string is similar regardless of the type of shape. Therefore, a developer thought it would be good to create a common factory function create for shapes as follows.
enum class ShapeFactory(private val tag: String) {
CIRCLE("circle") {
override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape =
Circle(param1, param2, param3)
},
RECTANGLE("rectangle") {
override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape {
requireNotNull(param4)
return Rectangle(param1, param2, param3, param4)
}
},
LINE("line") {
override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape {
requireNotNull(param4)
return Line(param1, param2, param3, param4)
}
};
abstract fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape
companion object {
val TAG_TO_FACTORY_MAP: Map<String, ShapeFactory> =
ShapeFactory.entries.associateBy(ShapeFactory::tag)
}
}By using this factory and String.split, the code to interpret the string can be implemented as follows.
fun parseShape(shapeString: String): Shape? {
val params = shapeString.split(" ")
val factory = ShapeFactory.TAG_TO_FACTORY_MAP[params[0]]
?: return null
val param1 = params[1].toInt()
val param2 = params[2].toInt()
val param3 = params[3].toInt()
val param4 = params.getOrNull(4)?.toInt()
return factory.create(param1, param2, param3, param4)
}Are there any improvements to be made to this code?
Keep wildcards hidden
Basically, you should avoid overly generic arguments that can be interpreted in multiple ways. The meaning of the arguments in create varies completely depending on the type of shape. For example, param1 means the x-coordinate of the center for Circle, and the left coordinate for Rectangle. Such definitions of arguments that depend on other arguments or the state of the receiver are undesirable.
Problems caused by such arguments include the following:
- Flexibility: Adding, removing, or updating arguments becomes difficult. For example, if you try to add a shape that requires five arguments, you will need to update the
createfunction signature for other shapes as well. Additionally, if arguments of a different type than the current argument type (such asStringorenum) are needed, the definition becomes overly complex. - Robustness: If the number of arguments varies depending on the type of shape, code to verify the number of arguments is required. As such verification code increases, handling errors tends to become complicated. Especially, if unnecessary runtime errors are introduced, it greatly affects robustness.
To make this problem easier to imagine, let's assume adding a shape that displays a single-word text called "word label". The data for the word label consists of the top-left coordinates (x, y) and any string other than spaces.
label 10 20 wordThe corresponding data model would look like this.
class WordLabel(x: Int, y: Int, word: String): ShapeWhen trying to implement this parser, you must change the type of the create arguments. Depending on the shape, the type of param3 can be either Int or String. To handle both Int and String in param3, you could use Any, define a sealed class for param3, or use type parameters. However, each has its pros and cons, and the existing shape code will be affected.
This problem can be solved by placing the responsibility of "parsing the string" in the appropriate place. While the code to decompose the tag can be summarized as common logic, the part of interpreting the values should be strongly tied to each shape. In the following revised proposal, createByMatchResult has the argument group with the common meaning of "the part of the value with the tag removed".
enum class ShapeFactory(private val tag: String, private val parameterRegex: Regex) {
CIRCLE("circle", """(-?\d+) (-?\d+) (\d+)""".toRegex()) {
override fun createByMatchResult(group: List): Shape =
Circle(group[1].toInt(), group[2].toInt(), group[3].toInt())
},
RECTANGLE("rectangle", """(-?\d+) (-?\d+) (-?\d+) (-?\d+)""".toRegex()) {
override fun createByMatchResult(group: List): Shape =
Rectangle(group[1].toInt(), group[2].toInt(), group[3].toInt(), group[4].toInt())
},
LINE("line", """(-?\d+) (-?\d+) (-?\d+) (-?\d+)""".toRegex()) {
override fun createByMatchResult(group: List): Shape =
Line(group[1].toInt(), group[2].toInt(), group[3].toInt(), group[4].toInt())
};
fun create(parameterText: String): Shape? {
val matchResult = parameterRegex.matchEntire(parameterText)
?: return null
return createByMatchResult(matchResult.groupValues)
}
protected abstract fun createByMatchResult(group: List): Shape
companion object {
val TAG_TO_FACTORY_MAP: Map<String, ShapeFactory> =
ShapeFactory.entries.associateBy(ShapeFactory::tag)
}
}The code for interpreting the string using this factory is as follows.
fun parseShape(shapeString: String): Shape? {
val tagAndParams = shapeString.split(" ", limit = 2)
val factory = ShapeFactory.TAG_TO_FACTORY_MAP[tagAndParams[0]]
val params = tagAndParams.getOrNull(1).orEmpty()
return factory?.create(params)
}Although this approach still includes code like group[x].toInt(), the association with the corresponding regular expression groups (-?\d+) or (\d+) is clearer. Therefore, the possibility of misusing the arguments is reduced. Note that in this implementation, the decomposition of the tag and the testing of the regular expression are done outside of createByMatchResult, but it is also possible to perform these within createByMatchResult.
With this refactoring, parameterText can be considered to mean "the string of the data part (with the tag removed)". It is important to make arguments interpretable as a single entity.
In a nutshell
Avoid defining overly generic arguments that can be interpreted in multiple ways.
Keywords: parameter, semantics