Request for Code Review


#1

I've posted a question on codereview.stackexchange.com regarding my Kotlin code, but understandably few kotlin coders are on CodeReview. Therefore, I would like to request a code review. Tear it apart, criticize it, suggest improvements and emerging kotlin idioms that I may have missed.

Original Question:

For practice I decided that I wanted to write an animation class for SpriteSheets. The problem is that a single Sprite can have different widths depending on the action they are drawn to act. A tool called Sprite Sheet Packer creates a Map file documenting the index and bounds of each sprite. This file looks a little like this:

0 = 157 184 123 189

1 = 187 0 177 183

2 = 402 181 105 189

3 = 495 0 111 180

4 = 710 0 102 171

walking = 1 2 3 4

Note that the walking = 1 2 3 4 is actually not part of the map file, but I want to add some extra information, such as which animation is comprised of which frames. This I convert into the following yaml:

  Frames:
  - Index: 0
  Bounds: [157, 184, 123, 189]
  - Index: 1
  Bounds: [187, 0, 177, 183]
  - Index: 2
  Bounds: [402, 181, 105, 189]
  - Index: 3
  Bounds: [495, 0, 111, 180]
  - Index: 4
  Bounds: [710, 0, 102, 171]
  Animations:
  - Name: walking
  Frames: [1, 2, 3, 4]

This is my code:

data class TextParseException(message: String = "", cause: Throwable? = null): RuntimeException(message, cause)

/**

  • <h2>Converts a Metadata .txt file into a .yaml file.</h2>
  • <b>Format of a metadata text file:</b>
  • <pre>
  •   <index>: <x> <y> <width> <height>
  •   …
  •   <name>: <frame1> <frame2> … <frameN>
  •   …
  • </pre>
  • <p>Be aware that a blank line (between … and <name>) is the separator between
  • frame and animation entries. Trailing newspaces are ok, but any others will break
  • the parsing.</p>
  • <b>Output format:</b>
  • <pre>
  •   Frames:
  •  &nbsp;&nbsp;- Index: &lt;index&gt;
    

*          Bounds: [<x>, <y>, <width>, <height>]

  •  &nbsp;&nbsp;...
    
  •   Animations:
  •  &nbsp;&nbsp;- Name: &lt;name&lt;
    

*          Frames: [<frame1>, <frame2>, …, <frameN>

  • </pre>

  • @param metadata path to metadata file for parsing

  • @returns formatted .yaml string of metadata contents

  • @throws TextParseException if an error occurs parsing a frame or animation entry

  • @throws IllegalArgumentException if the metadata file does not exist

  • @throws IOException if the file could not be accessed or read
    */
    fun convertToYaml(metadata: Path): String {
      Preconditions.checkArgument(Files.exists(metadata),
                               “The file ${metadata.getFileName()} does not exist”)

      val lines = Files.readAllLines(metadata, StandardCharsets.UTF_8).map { it.trim() }
      if (lines.all { it.isEmpty() })
      return “”

      val yamlFrameEntries = gatherRawFrameEntries(lines)
                               .map(::split)
                               .map(::toFrameEntry)
                               .sortBy { it.index }
                               .map(::createYamlFrameEntry)

      val yamlAnimationEntries = gatherRawAnimationEntries(lines)
                               .map(::split)
                               .map(::toAnimationEntry)
                               .map(::createYamlAnimationEntry)

      return buildCompleteYaml(yamlFrameEntries, yamlAnimationEntries)
    }

private fun gatherRawFrameEntries(lines: List<String>): List<String> {
  return lines.takeWhile { it.isNotEmpty() }
}

private fun gatherRawAnimationEntries(lines: List<String>): List<String> {
  return lines.dropWhile { it.isNotEmpty() }.drop(1).filter { it.isNotEmpty() }
}

private fun split(line: String): List<String> {
  val normalizedEntry = line.replace("=", " ")
  val parts = Splitter.on(’ ')!!
                   .trimResults()!!
                   .omitEmptyStrings()!!
                   .split(normalizedEntry)!!

  return parts.toList()
}

private data class FrameEntry(val index: Int, val bounds: List<Int>)
private data class AnimationEntry(val name: String, val frames: List<Int>)

private fun toFrameEntry(entryPieces: List<String>): FrameEntry {
  try {
  val frameIndex  = entryPieces.first!!
  val frameBounds = entryPieces.drop(1).map { it.toInt() }

  &nbsp;&nbsp;return FrameEntry(frameIndex.toInt(), frameBounds)

  } catch (e: Exception) {
  throw TextParseException(“Could not parse a Frame entry.”, e)
  }
}

private fun toAnimationEntry(entryPieces: List<String>): AnimationEntry {
  try {
  val animationName   = entryPieces.first!!
  val animationFrames = entryPieces.drop(1).map { it.toInt() }

  &nbsp;&nbsp;return AnimationEntry(animationName, animationFrames)

  } catch (e: Exception) {
  throw TextParseException(“Could not parse an Animation entry.”, e)
  }
}

private fun createYamlFrameEntry(frameEntry: FrameEntry): String {
  val yamlFrameEntryBuilder = StringBuilder()

  yamlFrameEntryBuilder.append("  - Index: {frameEntry.index}n") &nbsp;&nbsp;yamlFrameEntryBuilder.append(" &nbsp;&nbsp;Bounds: [") &nbsp;&nbsp;yamlFrameEntryBuilder.append("{Joiner.on(", “)!!.join(frameEntry.bounds)}]n”)

  return yamlFrameEntryBuilder.toString()
}

private fun createYamlAnimationEntry(animationEntry: AnimationEntry): String {
  val yamlFrameEntryBuilder = StringBuilder()

  yamlFrameEntryBuilder.append("  - Name: {animationEntry.name}n") &nbsp;&nbsp;yamlFrameEntryBuilder.append(" &nbsp;&nbsp;Frames: [") &nbsp;&nbsp;yamlFrameEntryBuilder.append("{Joiner.on(", “)!!.join(animationEntry.frames)}]n”)

  return yamlFrameEntryBuilder.toString()
}

private fun buildCompleteYaml(yamlFrameEntries: List<String>,
                           yamlAnimationEntries: List<String>): String {
  val completeYamlBuilder = StringBuilder()

  appendYamlEntries(completeYamlBuilder, “Frames:”, yamlFrameEntries)
  if (yamlAnimationEntries.isNotEmpty())
  appendYamlEntries(completeYamlBuilder, “Animations:”, yamlAnimationEntries)

  return completeYamlBuilder.toString()
}

private fun appendYamlEntries(builder: StringBuilder,
                           sectionName: String,
                           entries: List<String>) {
  builder.append("${sectionName}n")
  entries.forEach { builder.append(it) }
}


These are my tests:

class ConverterSpec: Spek() {{   given("A .txt -> .yaml converter") {   on("loading a non-existent file") {            it("throws an IllegalArgumentException") {            failsWith(javaClass<IllegalArgumentException>()) {                    convertToYaml(Paths.get("Missing.txt")!!)            }            }   }

  &nbsp;&nbsp;on("loading an empty file") {

           val emptyFileUrl = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/Empty.txt”)!!

           it(“produces empty yaml”) {
           val output = convertToYaml(Paths.get(emptyFileUrl.toURI())!!)

           assertTrue(output.isEmpty(),
                           “YAML output not empty, but:  $output”)
           }
  }

  &nbsp;&nbsp;on("loading file with on frame entry") {

           val frameFileUrl = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/Frame.txt”)!!

           it(“produces corresponding yaml for frame”) {
           val expected = “Frames:n” +
                           "  - Index: 0n" +
                           "   Bounds: [157, 184, 123, 189]n"

           val output = convertToYaml(Paths.get(frameFileUrl.toURI())!!)

           assertEquals(expected, output,
                           “The YAML did not conform to the expected output.”)
           }
  }

  &nbsp;&nbsp;on("loading file with multiple frame entries") {

           val framesFileUrl = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/Frames.txt”)!!

           it(“produces corresponding yaml for all frames”) {
           val expected = “Frames:n” +
                           "  - Index: 0n" +
                           "   Bounds: [157, 184, 123, 189]n" +
                           "  - Index: 1n" +
                           "   Bounds: [187, 0, 177, 183]n" +
                           "  - Index: 2n" +
                           "   Bounds: [402, 181, 105, 189]n"

           val output = convertToYaml(Paths.get(framesFileUrl.toURI())!!)

           assertEquals(expected, output,
                           “The YAML did not conform to the expected output.”)
           }
  }

  &nbsp;&nbsp;on("loading file with frames and animations") {

           val frameAnimationFileUrl = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/FrameAnimation.txt”)!!

           it(“produces corresponding yaml incorporating an animation name and number of frames”) {
           val expected = “Frames:n” +
                           "  - Index: 0n" +
                           "   Bounds: [157, 184, 123, 189]n" +
                           "  - Index: 1n" +
                           "   Bounds: [187, 0, 177, 183]n" +
                           "  - Index: 2n" +
                           "   Bounds: [402, 181, 105, 189]n" +
                           “Animations:n” +
                           "  - Name: Walkingn" +
                           "   Frames: [0, 1, 2]n"

           val output = convertToYaml(Paths.get(frameAnimationFileUrl.toURI())!!)

           assertEquals(expected, output,
                           “The YAML did not conform to the expected output.nPerhaps the Animations section is malformed?”)
           }
  }

  &nbsp;&nbsp;on("loading file with no frames but animations") {

           val onlyAnimationFileUrl = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/OnlyAnimation.txt”)!!

           it(“throws a TextParseException”) {
           failsWith(javaClass<TextParseException>()) {
                   convertToYaml(Paths.get(onlyAnimationFileUrl.toURI())!!)
           }
           }
  }

  &nbsp;&nbsp;on("loading file with trailing newlines") {

           val trailingNewlines = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/TrailingNewlines.txt”)!!

           it(“loads normally and ignores trailing newlines”) {
           val expected = “Frames:n” +
                           "  - Index: 0n" +
                           "   Bounds: [157, 184, 123, 189]n" +
                           "  - Index: 1n" +
                           "   Bounds: [187, 0, 177, 183]n" +
                           "  - Index: 2n" +
                           "   Bounds: [402, 181, 105, 189]n" +
                           “Animations:n” +
                           "  - Name: Walkingn" +
                           "   Frames: [0, 1, 2]n"

           val output = convertToYaml(Paths.get(trailingNewlines.toURI())!!)

           assertEquals(expected, output)
           }
  }

  &nbsp;&nbsp;on("Loading a file with unnatural ordering") {

           val unnaturalOrdering = javaClass<ConverterSpec>().getClassLoader()!!.getResource(“converter/UnsortedFrames.txt”)!!

           it(“loads and sorts frames into natural order”) {
           val expected = “Frames:n” +
                           "  - Index: 0n" +
                           "   Bounds: [157, 184, 123, 189]n" +
                           "  - Index: 1n" +
                           "   Bounds: [187, 0, 177, 183]n" +
                           "  - Index: 2n" +
                           "   Bounds: [402, 181, 105, 189]n" +
                           "  - Index: 10n" +
                           "   Bounds: [187, 0, 177, 183]n"

           val output = convertToYaml(Paths.get(unnaturalOrdering.toURI())!!)

           assertEquals(expected, output)
           }
  }
  }
}}


#2

Do you have this code in an Open Source respository (namely Github)?


#3

Hello Mario,

actually, I put off creating the repository because I felt the code was still too infantile. I’ve remedied that now, it’s the SpriteSheetProcessor repository.


#4

Done https://github.com/SoulBeaver/SpriteSheetProcessor/pull/1