2
\$\begingroup\$

I'm learning kotlin and java. As a code-kata I wrote the roman to decimal converter. I'd be very grateful for code review. I was trying to keep things simple and funcional. I'd especially appreciate any comments on the functional style (e.g. the non-purity of some mappings).

Converter tests: package roman

import org.hamcrest.core.Is.`is`
import org.junit.Assert.assertThat
import org.junit.Test
import roman.RomanNumber.*
import kotlin.test.assertTrue

/**
 * Created by skramer on 7/25/16.
 * - <s>single values, ROMAN_ONE, V, X, L, C, D, M</s>
 * - <s>concatenated single values II, III, XX, DD</s>
 * - <s>illegal concatenations IIII, XXXX</s>
 * - <s>mixed simple additive concatenations XI, VI, LX</s>
 * - <s>MMMDCCCLXXXVIII == 3888</s>
 * - <s>mixed simple subtractive concatenations IX, XL</s>
 * - <s>illegal subtractive concatenations e.g. IM, XXXL, VX, XXXXL, LXXX, LXXXX, IIX</s>
 * - <s>acceptance test: 1999</s>
 * - <s>boundary values: 1, 3000, 1000, 666, 999, 888<s>
 * - <s>convert from string? (case sensitivity, empty string, unknown letter)</s>
 */

class RomanNumeralsTest {

    @Test
    fun numericalValueOfSingleLetterIIsRecognized() {
        assertThat(romanToDecimal(ROMAN_ONE), `is`(1))
    }

    @Test
    fun numericalValueOfSingleLetterVIsRecognized() {
        assertThat(romanToDecimal(ROMAN_FIVE), `is`(5))
    }

    @Test
    fun numericalValueOfSingleLetterXIsRecognized() {
        assertThat(romanToDecimal(ROMAN_TEN), `is`(10))
    }

    @Test
    fun numericalValueOfSingleLetterLIsRecognized() {
        assertThat(romanToDecimal(ROMAN_FIFTY), `is`(50))
    }

    @Test
    fun numericalValueOfSingleLetterCIsRecognized() {
        assertThat(romanToDecimal(ROMAN_HUNDRED), `is`(100))
    }

    @Test
    fun numericalValueOfSingleLetterDIsRecognized() {
        assertThat(romanToDecimal(ROMAN_FIVE_HUNDRED), `is`(500))
    }

    @Test
    fun numericalValueOfSingleLetterMIsRecognized() {
        assertThat(romanToDecimal(ROMAN_THOUSAND), `is`(1000))
    }

    @Test
    fun twoRomanOnesCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_ONE, ROMAN_ONE), `is`(2))
    }

    @Test
    fun threeRomanOnesCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_ONE, ROMAN_ONE, ROMAN_ONE), `is`(3))
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfFourRomanOnesIsRejected() {
        romanToDecimal(ROMAN_ONE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE)
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfTwoFivesIsRejected() {
        romanToDecimal(ROMAN_FIVE, ROMAN_FIVE)
    }

    @Test
    fun twoRomanTensCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_TEN), `is`(20))
    }

    @Test
    fun threeRomanTensCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_TEN, ROMAN_TEN), `is`(30))
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfFourTensIsRejected() {
        romanToDecimal(ROMAN_TEN, ROMAN_TEN, ROMAN_TEN, ROMAN_TEN)
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfTwoFiftiesIsRejected() {
        romanToDecimal(ROMAN_FIFTY, ROMAN_FIFTY)
    }

    @Test
    fun twoRomanHundredsCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_HUNDRED, ROMAN_HUNDRED), `is`(200))
    }

    @Test
    fun threeRomanHundredsCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_HUNDRED, ROMAN_HUNDRED, ROMAN_HUNDRED), `is`(300))
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfFourHundredsIsRejected() {
        romanToDecimal(ROMAN_HUNDRED, ROMAN_HUNDRED, ROMAN_HUNDRED, ROMAN_HUNDRED)
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfTwoFiveHundredsIsRejected() {
        romanToDecimal(ROMAN_FIVE_HUNDRED, ROMAN_FIVE_HUNDRED)
    }

    @Test
    fun twoRomanThousandsCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_THOUSAND, ROMAN_THOUSAND), `is`(2000))
    }

    @Test
    fun threeRomanThousandsCanBeConcatenated() {
        assertThat(romanToDecimal(ROMAN_THOUSAND, ROMAN_THOUSAND, ROMAN_THOUSAND), `is`(3000))
    }

    @Test(expected = IllegalArgumentException::class)
    fun concatenationOfFourThousandsIsRejected() {
        romanToDecimal(ROMAN_THOUSAND, ROMAN_THOUSAND, ROMAN_THOUSAND, ROMAN_THOUSAND)
    }

    @Test
    fun additiveConcatenationOfFiveAndOne() {
        assertThat(romanToDecimal(ROMAN_FIVE, ROMAN_ONE), `is`(6))
        assertThat(romanToDecimal(ROMAN_FIVE, ROMAN_ONE, ROMAN_ONE), `is`(7))
        assertThat(romanToDecimal(ROMAN_FIVE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE), `is`(8))
    }

    @Test(expected = IllegalArgumentException::class)
    fun additiveConcatenationOfFiveAndFourOnesIsRejected() {
        romanToDecimal(ROMAN_FIVE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE)
    }

    @Test
    fun additiveConcatenationOfTenAndFive() {
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_FIVE), `is`(15))
    }

    @Test
    fun additiveConcatenationOfTenFiveAndThreeOnes() {
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_FIVE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE), `is`(18))
    }

    @Test
    fun maximalAdditiveConcatenation() {
        assertThat(romanToDecimal(ROMAN_THOUSAND, ROMAN_THOUSAND, ROMAN_THOUSAND, ROMAN_FIVE_HUNDRED,
                                  ROMAN_HUNDRED, ROMAN_HUNDRED, ROMAN_HUNDRED,
                                  ROMAN_FIFTY, ROMAN_TEN, ROMAN_TEN, ROMAN_TEN,
                                  ROMAN_FIVE, ROMAN_ONE, ROMAN_ONE, ROMAN_ONE), `is`(3888))
    }

    @Test
    fun subtractiveConcatenationWithSingleValue() {
        assertThat(romanToDecimal(ROMAN_ONE, ROMAN_FIVE), `is`(4))
        assertThat(romanToDecimal(ROMAN_ONE, ROMAN_TEN), `is`(9))
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_FIFTY), `is`(40))
        assertThat(romanToDecimal(ROMAN_TEN, ROMAN_HUNDRED), `is`(90))
        assertThat(romanToDecimal(ROMAN_HUNDRED, ROMAN_FIVE_HUNDRED), `is`(400))
        assertThat(romanToDecimal(ROMAN_HUNDRED, ROMAN_THOUSAND), `is`(900))
    }

    @Test
    fun acceptanceTest() {
        assertThat(romanToDecimal(ROMAN_THOUSAND, ROMAN_HUNDRED, ROMAN_THOUSAND,
                                  ROMAN_TEN, ROMAN_HUNDRED, ROMAN_ONE, ROMAN_TEN), `is`(1999))
    }

    @Test
    fun remainingBoundaryValues() {
        assertThat(romanToDecimal(ROMAN_FIVE_HUNDRED, ROMAN_HUNDRED, ROMAN_FIFTY, ROMAN_TEN, ROMAN_FIVE, ROMAN_ONE),
                   `is`(666))

        assertThat(romanToDecimal(ROMAN_HUNDRED, ROMAN_THOUSAND,
                                  ROMAN_TEN, ROMAN_HUNDRED, ROMAN_ONE, ROMAN_TEN), `is`(999))
    }

    @Test
    fun convertFromString() {
        assertThat(romanToDecimal("MMMDCCCLXXXVIII"), `is`(3888))
        assertThat(romanToDecimal("MMM"), `is`(3000))
        assertThat(romanToDecimal("IX"), `is`(9))
        assertThat(romanToDecimal("CCXC"), `is`(290))
    }

    @Test
    fun emptyString() {
        assertThat(romanToDecimal(""), `is`(0))
    }

    @Test
    fun caseSensitivityIsIgnored() {
        assertThat(romanToDecimal("mmmdccclxxxviii"), `is`(3888))
    }

    @Test(expected = IllegalArgumentException::class)
    fun unknownLetterIsRejected() {
        romanToDecimal("Z")
    }

    @Test
    fun illegalSubtractiveConcatenationsAreRejected() {
        assertTrue(throwsIllegalArgumentException("IM"))
        assertTrue(throwsIllegalArgumentException("LM"))
        assertTrue(throwsIllegalArgumentException("XD"))
        assertTrue(throwsIllegalArgumentException("ID"))
        assertTrue(throwsIllegalArgumentException("LM"))
    }

    private fun throwsIllegalArgumentException(romanString: String): Boolean {
        try {
            romanToDecimal(romanString)
            return false
        } catch (_ : IllegalArgumentException) {
            return true
        }
    }

    @Test
    fun specificSubtractiveConcatenationsAreHandled() {
        assertTrue(throwsIllegalArgumentException("VX"))
    }
}

Converter:

package roman

import algorithm.groupAdjacentBy
import roman.RomanNumber.*

enum class RomanNumber(val number: Int, val char: Char, val max_repetition: Int = 3) {

    ROMAN_ONE(1, 'I'),

    ROMAN_FIVE(5, 'V', max_repetition = 1),

    ROMAN_TEN(10, 'X'),

    ROMAN_FIFTY(50, 'L', max_repetition = 1),

    ROMAN_HUNDRED(100, 'C'),

    ROMAN_FIVE_HUNDRED(500, 'D', max_repetition = 1),

    ROMAN_THOUSAND(1000, 'M');

}

private fun getAlloweSubtractionConcatenations(): Map<RomanNumber, RomanNumber> {
    return mapOf(ROMAN_FIVE to ROMAN_ONE, ROMAN_TEN to ROMAN_ONE, ROMAN_FIFTY to ROMAN_TEN, ROMAN_HUNDRED to ROMAN_TEN,
                 ROMAN_FIVE_HUNDRED to ROMAN_HUNDRED, ROMAN_THOUSAND to ROMAN_HUNDRED)
}

fun romanToDecimal(romanNumber: RomanNumber, vararg romanNumbers: RomanNumber): Int {
    return romanToDecimal(listOf(romanNumber, *romanNumbers))
}

fun romanToDecimal(romanNumberString: String): Int {
    val romanNumberList = romanNumberString
            .map { it.toUpperCase() }
            .map { letter -> values().find { enum_value -> enum_value.char == letter } }
            .toList()

    return romanToDecimal(romanNumberList.requireNoNulls())
}

fun romanToDecimal(romanNumberList: List<RomanNumber>): Int {
    checkPreconditions(romanNumberList)

    val rawConvertedValues = convertRomanNumbersToDecimal(romanNumberList)
    return parseRawConvertedValues(rawConvertedValues)
}

private fun parseRawConvertedValues(rawConvertedValues: List<Int>): Int {
    val increasingValuesGroups = splitIntoGroupsOfIncreasingValues(rawConvertedValues)
    val increasingValuesGroupsFinalValues = calculateFinalValuesOfIncreasingValuesGroups(increasingValuesGroups)
    return increasingValuesGroupsFinalValues.sum()
}

private fun calculateFinalValuesOfIncreasingValuesGroups(increasingValuesGroups: List<List<Int>>): List<Int> {
    return increasingValuesGroups.map { valuesList -> return@map calculateGroupFinalValue(valuesList) }
}

private fun calculateGroupFinalValue(valuesList: List<Int>): Int {
    if (!isIncreasingGroupSubtractive(valuesList)) {
        return valuesList.sum()
    }

    return calculateFinalValueForSubtractiveGroup(valuesList)
}

private fun calculateFinalValueForSubtractiveGroup(valuesList: List<Int>) = 2 * valuesList.last() - valuesList.sum()

private fun <E> isIncreasingGroupSubtractive(valuesList: List<E>) = valuesList.first() != valuesList.last()

private fun <E : Comparable<E>> splitIntoGroupsOfIncreasingValues(
        rawConvertedValues: List<E>) = rawConvertedValues.groupAdjacentBy { lhs, rhs -> lhs.compareTo(rhs) <= 0 }

private fun checkPreconditions(romanNumbers: List<RomanNumber>) {
    checkLetterRepetitions(romanNumbers)
    checkLetterWeighting(romanNumbers)
}

fun checkLetterWeighting(romanNumbers: List<RomanNumber>) {
    val groupedIncreasingRomanValues: List<List<RomanNumber>> = splitIntoGroupsOfIncreasingValues(romanNumbers)
    val offendingRomanNumbers = groupedIncreasingRomanValues.filter { isIncreasingGroupSubtractive(it) }
            .filter { list -> list.size > 2 || getAlloweSubtractionConcatenations()[list.last()] != list.first()  }

    if (!offendingRomanNumbers.isEmpty()) {
        val sb = StringBuilder()
        offendingRomanNumbers.forEach { sb.append(it) }
        throw IllegalArgumentException("Illegal letter weighting: ${sb.toString()}")
    }
}

private fun checkLetterRepetitions(romanNumbers: List<RomanNumber>) {
    val groupedRomanNumbers = getGroupsOfEqualNumbers(romanNumbers)
    val offendingLettersGroups = getGroupsWithTooManyElements(groupedRomanNumbers)

    if (!offendingLettersGroups.isEmpty()) {
        val sb = StringBuilder()
        offendingLettersGroups.forEach { list ->
            sb.append(
                    "Concatenation of ${list.size} characters ${list[0].name} violates maximum repetition count ${list[0].max_repetition}")
        }

        throw IllegalArgumentException(sb.toString())

    }
}

private fun getGroupsWithTooManyElements(
        groupedRomanNumbers: List<List<RomanNumber>>) = groupedRomanNumbers.filter { list -> list.size > list[0].max_repetition }

private fun getGroupsOfEqualNumbers(
        romanNumbers: List<RomanNumber>) = romanNumbers.groupAdjacentBy { lhs, rhs -> lhs.number == rhs.number }

private fun convertRomanNumbersToDecimal(romanNumber: List<RomanNumber>): List<Int> = romanNumber.map(
        ::convertSingleRomanNumberToDecimal)

private fun convertSingleRomanNumberToDecimal(romanNumber: RomanNumber): Int = romanNumber.number

groupAdjacentBy tests:

package algorithm

import org.hamcrest.core.Is.`is`
import org.junit.Assert.assertThat
import org.junit.Test

/**
 * Created by skramer on 7/30/16.
 * Tests for groupAdjacentBy algorithm.
 * - <s>single element</s>
 * - <s>two equal elements</s>
 * - <s>two different elements</s>
 * - <s>all elements different return list of lists, each with one element</s>
 * - <s>empty list</s>
 * - <s>singleton list</s>
 * - <s>list with multiple, not subsequent occurrences of an element</s>
 */

class GroupAdjacentTest() {
    @Test
    fun singleElementIsPlacedInSinglePair() {
        assertThat(listOf(1).groupAdjacentBy(Int::equals), `is`(listOf(listOf(1))))
    }

    @Test
    fun twoEqualElementsReturnListWithOnlyOnePair() {
        assertThat(listOf(1, 1).groupAdjacentBy(Int::equals), `is`(listOf(listOf(1, 1))))
    }

    @Test
    fun multipleEqualElementsReturnListWithOnlyOnePair() {
        val equalElements = (1 .. 100).map { 1 }.toList()
        assertThat(equalElements.groupAdjacentBy(Int::equals), `is`(listOf(equalElements)))
    }

    @Test
    fun twoDifferentElementsReturnPairEach() {
        assertThat(listOf(1, 2).groupAdjacentBy(Int::equals), `is`(listOf(listOf(1), listOf(2))))
    }

    @Test
    fun allElementsDifferentReturnListForEachEntry() {
        val inputArray = (1..100).toList()
        val expectedOutput = (1..100).map { listOf(it) }
        assertThat(inputArray.groupAdjacentBy(Int::equals), `is`(expectedOutput))
    }

    @Test
    fun multipleNotSubSequentOccurrencesOfDifferentElements() {
        val inputArray = arrayListOf<Int>()
        for (i in 1..10) {
            inputArray.addAll(1..10)
        }
        assertThat(inputArray.size, `is`(100))

        val expectedOutput = inputArray.map { listOf(it) }

        assertThat(inputArray.groupAdjacentBy(Int::equals), `is`(expectedOutput.toList()))
    }

    @Test
    fun multipleNotSubSequentOccurrencesOfDifferentElementsWithRepetitions() {
        val inputArray = arrayListOf<Int>()
        (1..10).forEach { inputArray.addAll(listOf(it, it, it)) }

        val expected = (1..10).map { listOf(it, it, it) }.toList()
        assertThat(inputArray.groupAdjacentBy(Int::equals), `is`(expected.toList()))
    }

    @Test
    fun emptyList() {
        val expected: List<List<Int>> = listOf()
        val actual: List<List<Int>> = listOf<Int>().groupAdjacentBy(Int::equals)
        assertThat(actual, `is`(expected))
    }

    val lessThanPredicate: (Int, Int) -> Boolean = { left, right -> left.compareTo(right) < 0 }
    val lessThanOrEqualsPredicate: (Int, Int) -> Boolean = { left, right -> left.compareTo(right) <= 0 }

    @Test
    fun singletonList() {
        assertThat(listOf(1).groupAdjacentBy ( Int::equals ), `is`(listOf(listOf(1))))
        assertThat(listOf(1).groupAdjacentBy ( lessThanPredicate ), `is`(listOf(listOf(1))))
    }

    @Test
    fun groupElementsUntilIncreasingTrendIsOver() {
        val input = listOf(1, 2, 3, 0, 5, 6, 0, 8, 9)
        assertThat(input.groupAdjacentBy(lessThanPredicate), `is`(listOf(listOf(1, 2, 3), listOf(0, 5, 6), listOf(0, 8, 9))))
    }

    @Test
    fun groupElementsUntilIncreasingTrendIsOverWithRepetitiveElements() {
        val input = listOf(1, 1, 2, 2, 3, 3, 0, 5, 6, 0, 8, 9)
        assertThat(input.groupAdjacentBy(lessThanOrEqualsPredicate), `is`(listOf(listOf(1, 1, 2, 2, 3, 3), listOf(0, 5, 6), listOf(0, 8, 9))))
    }
}

groupAdjacent algorithm:

package algorithm

import java.util.*

/**
 * Created by skramer on 7/30/16.
 *
 * groupAdjacentBy algorithm. Uses the provided predicate to group subsequent entries in the collection.
 */

fun <E> List<E>.groupAdjacentBy(binaryPredicate: (E, E) -> Boolean): List<List<E>> {
    val result = arrayListOf<List<E>>()

    var currentElementIndex = 0

    while (currentElementIndex < this.size) {
        var currentElement = this[currentElementIndex]

        val list = subList(currentElementIndex + 1, this.size).takeWhile {
            val predicateResult = binaryPredicate(currentElement, it)
            currentElement = it
            return@takeWhile predicateResult
        }

        result.add(getPartialResultAsList(currentElementIndex, list))

        currentElementIndex += 1 + list.size
    }

    return result
}

private fun <E> List<E>.getPartialResultAsList(currentElementIndex: Int, list: List<E>): ArrayList<E> {
    val partialResult = arrayListOf(this[currentElementIndex])
    partialResult.addAll(list)
    return partialResult
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.