add self-review comments

Author: fractalwrench

TODO.md

@@ -6,6 +6,8 @@ Ordered roughly in priority:
 - Add more real world payloads
 - Symbol pool for class names
 - General cleanup of weird/disgusting code areas
+- Address all FIXME/TODOs
+- Write Contributing guide
 
 - Make website tool prettier
 - Throttle website tool/prepare for HN

cmdline/src/main/kotlin/com/fractalwrench/json2kotlin/App.kt

@@ -22,7 +22,7 @@ fun main(args: Array<String>) {
 
             if (inputFile.exists()) {
                 val outputFile = findOutputFile(inputFile)
-                KotlinJsonConverter().convert(inputFile.readText(), outputFile.outputStream(), ConversionArgs())
+                Kotlin2JsonConverter().convert(inputFile.readText(), outputFile.outputStream(), ConversionArgs())
                 println("Generated source available at '$outputFile'")
             } else {
                 println("Failed to find file '$inputFile'")

core/src/main/kotlin/com/fractalwrench/json2kotlin/ClassTypeHolder.kt

@@ -4,7 +4,7 @@ import com.squareup.kotlinpoet.*
 import java.util.*
 
 
-internal class ClassTypeHolder(val delegate: SourceBuildDelegate) : TraversalDelegate {
+internal class ClassTypeHolder(val delegate: SourceBuildDelegate) : TraversalDelegate { // TODO rename, bad ontology
 
     internal val stack = Stack<TypeSpec>()
     private val jsonProcessor = JsonProcessor()
@@ -13,9 +13,9 @@ internal class ClassTypeHolder(val delegate: SourceBuildDelegate) : TraversalDel
     /**
      * Processes a single level in the tree
      */
-    override fun processTreeLevel(levelQueue: LinkedList<TypedJsonElement>) {
+    override fun processTreeLevel(levelQueue: LinkedList<TypedJsonElement>) { // FIXME not ll, generify?
         val fieldValues = levelQueue.filter { it.isJsonObject }.toMutableList()
-        fieldValues.forEach { println(it) }
+        fieldValues.forEach { println(it) } // TODO add verbose flag to config, default to false
 
         jsonFieldGrouper.groupCommonFieldValues(fieldValues)
                 .flatMap { convertFieldsToTypes(it) }
@@ -40,17 +40,17 @@ internal class ClassTypeHolder(val delegate: SourceBuildDelegate) : TraversalDel
 
         return commonElements.filterNot { // reuse any types which already exist in the map
             val containsValue = jsonProcessor.jsonElementMap.containsValue(classType)
-            jsonProcessor.jsonElementMap.put(it.jsonElement, classType) // FIXME weird
+            jsonProcessor.jsonElementMap.put(it.jsonElement, classType) // FIXME feels weird
             containsValue
         }.map { classType }
     }
 
     private fun buildClass(commonElements: List<TypedJsonElement>, fields: Collection<String>): TypeSpec.Builder {
-        val identifier = commonElements.last().kotlinIdentifier
+        val identifier = commonElements.last().kotlinIdentifier // FIXME should only pass in one if that's all that's needed!
         val classBuilder = TypeSpec.classBuilder(identifier.capitalize()) // FIXME check symbol pool!
         val constructor = FunSpec.constructorBuilder()
 
-        if (fields.isEmpty()) {
+        if (fields.isEmpty()) { // FIXME misses delegate!
             return classBuilder
         }
 
@@ -67,7 +67,7 @@ internal class ClassTypeHolder(val delegate: SourceBuildDelegate) : TraversalDel
         val sanitisedName = field.toKotlinIdentifier() // FIXME should be done before this
         val typeName = fieldTypeMap[field]
         val initializer = PropertySpec.builder(sanitisedName, typeName!!).initializer(sanitisedName)
-        delegate.prepareClassProperty(initializer, sanitisedName, field) // FIXME pass in original name
+        delegate.prepareClassProperty(initializer, sanitisedName, field)
         classBuilder.addProperty(initializer.build())
         constructor.addParameter(sanitisedName, typeName)
     }

core/src/main/kotlin/com/fractalwrench/json2kotlin/ConversionArgs.kt

@@ -1,3 +1,3 @@
 package com.fractalwrench.json2kotlin
 
-class ConversionArgs(val rootClassName: String = "Example")
+class ConversionArgs(val rootClassName: String = "Example") // TODO support a few other options (package name, build delegate)

core/src/main/kotlin/com/fractalwrench/json2kotlin/GsonBuildDelegate.kt

@@ -7,6 +7,11 @@ import com.squareup.kotlinpoet.TypeSpec
 
 class GsonBuildDelegate: SourceBuildDelegate {
 
+    // TODO pattern compilation
+    // FIXME pattern super hacky
+    // TODO should pass TypedJsonElement (and as much info as possible,
+    // maybe in a wrapper class e.g. PropBuildParams/ClassBuildParams)
+
     override fun prepareClassProperty(propertyBuilder: PropertySpec.Builder,
                                       kotlinIdentifier: String,
                                       jsonKey: String?) {

core/src/main/kotlin/com/fractalwrench/json2kotlin/JsonFieldGrouper.kt

@@ -35,6 +35,8 @@ internal class JsonFieldGrouper {
         }
     }
 
+    // TODO this should be exposed as a constructor parameter, will allow greater extensibility
+
     /**
      * Determines whether two JSON Objects on the same level of a JSON tree share the same class type.
      *
@@ -52,5 +54,5 @@ internal class JsonFieldGrouper {
         val keySize = if (lhsKeys.size > rhsKeys.size) lhsKeys.size else rhsKeys.size
         val commonKeyCount = if (emptyClasses) 1 else lhsKeys.intersect(rhsKeys).size
         return (commonKeyCount * 5) >= keySize // at least a fifth of keys must match
-    }
+    } // FIXME should also consider relative size of objects (if lhs has 99 keys, and rhs has 1 key, then they shouldn't match)
 }

core/src/main/kotlin/com/fractalwrench/json2kotlin/JsonNames.kt

@@ -2,8 +2,66 @@ package com.fractalwrench.json2kotlin
 
 import com.google.gson.JsonElement
 
+// TODO move to a symbol pool class (or equivalent)
+
 internal fun nameForArrayField(sanitisedName: String) = "${sanitisedName}Array"
 
 internal fun nameForObjectInArray(it: IndexedValue<JsonElement>, sanitisedName: String): String {
     return if (it.index > 0) "$sanitisedName${it.index + 1}" else sanitisedName
 }
+
+
+// TODO move to a symbol pool class (or equivalent)
+
+// FIXME pattern compilation
+
+fun String.standardiseNewline(): String {
+    return this.replace("\r\n", "\n")
+}
+
+fun String.toKotlinIdentifier(): String {
+    return when {
+        KEYWORDS.contains(this) -> "`$this`" // escape
+        else -> {
+            val sanitisedOutput = this.replace("[^0-9A-Za-z_]+".toRegex(), "_")
+            val regex = "^[^A-Za-z_].*".toRegex()
+
+            when {
+                regex.matches(sanitisedOutput) -> "_$sanitisedOutput"
+                else -> sanitisedOutput
+            }
+        }
+    }
+}
+
+private val KEYWORDS = listOf(
+        "as",
+        "as?",
+        "break",
+        "class",
+        "continue",
+        "do",
+        "else",
+        "false",
+        "for",
+        "fun",
+        "if",
+        "in",
+        "!in",
+        "interface",
+        "is",
+        "!is",
+        "null",
+        "object",
+        "package",
+        "return",
+        "super",
+        "this",
+        "throw",
+        "try",
+        "typealias",
+        "val",
+        "var",
+        "when",
+        "while"
+)

core/src/main/kotlin/com/fractalwrench/json2kotlin/JsonProcessor.kt

@@ -9,10 +9,11 @@ import java.util.HashMap
 import java.util.HashSet
 
 
-internal class JsonProcessor {
+internal class JsonProcessor { // TODO crappy name
 
-    internal val jsonElementMap = HashMap<JsonElement, TypeSpec>()
+    internal val jsonElementMap = HashMap<JsonElement, TypeSpec>() // FIXME feels wrong having this exposed
 
+    // FIXME should take TypedJsonElement rather than String as a param!
     fun findDistinctTypesForFields(fields: Collection<String>,
                                    commonElements: List<TypedJsonElement>): Map<String, TypeName> {
         val fieldMap = HashMap<String, TypeName>()
@@ -31,6 +32,12 @@ internal class JsonProcessor {
         }.distinct()
     }
 
+
+
+    // TODO: refactor all the (simple) type deduction methods. They can take an extra Map parameter. This will greatly simplify any unit
+    // testing and pass Single-Responsibility test
+
+
     private fun typeForJsonField(jsonElement: JsonElement, key: String): TypeName {
         with(jsonElement) {
             return when {

core/src/main/kotlin/com/fractalwrench/json2kotlin/KotlinJsonConverter.kt renamed to core/src/main/kotlin/com/fractalwrench/json2kotlin/Kotlin2JsonConverter.kt

@@ -6,9 +6,12 @@ import java.io.OutputStream
 /**
  * Converts JSON to Kotlin
  */
-class KotlinJsonConverter {
+class Kotlin2JsonConverter(buildDelegate: SourceBuildDelegate = GsonBuildDelegate()) {
+
+    // TODO (general: update KDocs!)
+
+    // TODO expose grouping class as a parameter, add to config
 
-    private val buildDelegate: SourceBuildDelegate = GsonBuildDelegate()
     private val jsonReader = JsonReader(JsonParser())
     private val sourceFileWriter = SourceFileWriter()
     private val typeHolder = ClassTypeHolder(buildDelegate)
@@ -17,7 +20,7 @@ class KotlinJsonConverter {
     /**
      * Converts a JSON string to Kotlin, writing it to the OutputStream.
      */
-    fun convert(input: String, output: OutputStream, args: ConversionArgs) {
+    fun convert(input: String, output: OutputStream, args: ConversionArgs) { // FIXME should take an InputStream as input
         try {
             if (input.isEmpty()) {
                 throw IllegalArgumentException("Json input empty")

core/src/main/kotlin/com/fractalwrench/json2kotlin/ReverseJsonTreeTraverser.kt

@@ -13,13 +13,13 @@ internal class ReverseJsonTreeTraverser(delegate: TraversalDelegate) : Traversal
 
     private val bfsStack: Stack<TypedJsonElement> = Stack()
 
-    fun traverse(element: JsonElement, rootName: String) {
+    fun traverse(element: JsonElement, rootName: String) { // TODO should return a stack instead and not use a delegate
         buildStack(element, rootName)
         validateStackOrder()
         processQueue()
     }
 
-    private fun validateStackOrder() {
+    private fun validateStackOrder() { // TODO add to a test instead (perf)
         var maxLevel = 0
         bfsStack.forEach {
             if (maxLevel > it.level) {
@@ -56,7 +56,7 @@ internal class ReverseJsonTreeTraverser(delegate: TraversalDelegate) : Traversal
      * Processes JSON nodes in a reverse level order traversal,
      * by building class types for each level of the tree.
      */
-    private fun processQueue() {
+    private fun processQueue() { // TODO split into two separate classes, as separate responsibilities.
         var level = -1
         val levelQueue = LinkedList<TypedJsonElement>()
 
@@ -89,7 +89,7 @@ internal class ReverseJsonTreeTraverser(delegate: TraversalDelegate) : Traversal
     }
 
     private fun nameForArrayField(index: Int, identifier: String): String =
-            if (index == 0) identifier else "$identifier${index + 1}"
+            if (index == 0) identifier else "$identifier${index + 1}" // FIXME duplication
 
     private fun shouldAddToStack(element: JsonElement) = element.isJsonArray || element.isJsonObject
 

core/src/main/kotlin/com/fractalwrench/json2kotlin/SourceBuildDelegate.kt

@@ -3,7 +3,7 @@ package com.fractalwrench.json2kotlin
 import com.squareup.kotlinpoet.PropertySpec
 import com.squareup.kotlinpoet.TypeSpec
 
-interface SourceBuildDelegate {
+interface SourceBuildDelegate { // FIXME ensure names are correct, as this will be public API!
     fun prepareClassProperty(propertyBuilder: PropertySpec.Builder,
                              kotlinIdentifier: String,
                              jsonKey: String?)

core/src/main/kotlin/com/fractalwrench/json2kotlin/SourceFileWriter.kt

@@ -8,19 +8,19 @@ import java.util.*
 /**
  * Writes a collection of types to a source file OutputStream.
  */
-internal class SourceFileWriter {
+internal class SourceFileWriter { // TODO rename once functionality has changed
 
     /**
      * Writes a collection of types to a source file OutputStream.
      */
     fun writeSourceFile(stack: Stack<TypeSpec>, args: ConversionArgs, output: OutputStream) {
-        val sourceFile = FileSpec.builder("", args.rootClassName)
+        val sourceFile = FileSpec.builder("", args.rootClassName) // FIXME set package name
 
         while (stack.isNotEmpty()) {
             sourceFile.addType(stack.pop())
         }
 
-        with(StringBuilder()) {
+        with(StringBuilder()) { // FIXME inefficient use of resources
             sourceFile.build().writeTo(this)
             output.write(toString().toByteArray())
         }

core/src/main/kotlin/com/fractalwrench/json2kotlin/StringExtensions.kt

@@ -2,6 +2,6 @@ package com.fractalwrench.json2kotlin
 
 import java.util.*
 
-interface TraversalDelegate {
+interface TraversalDelegate { // TODO remove
     fun processTreeLevel(levelQueue: LinkedList<TypedJsonElement>)
 }

core/src/main/kotlin/com/fractalwrench/json2kotlin/TraversalDelegate.kt

@@ -4,12 +4,12 @@ import com.google.gson.*
 import java.math.BigDecimal
 import java.math.BigInteger
 
-class TypedJsonElement : JsonElement {
+class TypedJsonElement : JsonElement { // TODO check visibility
 
     val jsonElement: JsonElement
     val jsonKey: String
     val level: Int
-    val kotlinIdentifier: String
+    val kotlinIdentifier: String // FIXME symbol pooling
 
     constructor(jsonElement: JsonElement, jsonKey: String, level: Int) : super() {
         this.jsonElement = jsonElement

core/src/main/kotlin/com/fractalwrench/json2kotlin/TypedJsonElement.kt

@@ -1,6 +1,5 @@
 package com.fractalwrench.json2kotlin
 
-import com.google.gson.JsonParser
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.Parameterized
@@ -10,7 +9,7 @@ import java.io.ByteArrayOutputStream
 class InvalidJsonConverterTest(val jsonFilename: String) {
 
     private val fileReader = ResourceFileReader()
-    private val jsonConverter = KotlinJsonConverter()
+    private val jsonConverter = Kotlin2JsonConverter()
 
     companion object {
         @JvmStatic

core/src/test/kotlin/com/fractalwrench/json2kotlin/InvalidJsonConverterTest.kt

@@ -12,7 +12,7 @@ import java.io.File
 open class JsonConverterTest(val expectedFilename: String, val jsonFilename: String) {
 
     private val fileReader = ResourceFileReader()
-    private val jsonConverter = KotlinJsonConverter()
+    private val jsonConverter = Kotlin2JsonConverter()
     internal lateinit var json: String
 
     @Before