problem-specifications icon indicating copy to clipboard operation
problem-specifications copied to clipboard

Transpose test cases inconsistent.

Open andsus opened this issue 4 years ago • 4 comments

As the name exercise Transpose, the exercise is similar to Matrix exercise, but use character in the matrix cell.

Discrepancy between test cases, in Kotlin but the issues are for all tracks

  • Given 2 rows with uneven length
  • The solutions should pad the length to longest length with space, and transpose it
  • Expected, the transposed chars is a pair of transposed , exactly like second line longer than first line where the last pair is " ." , space followed by dot.
  • Bugs on first line longer than second line case, where last pair only expected single char.
  • Bugs on mixed line length test case as well. The expected transposed chars, should have all 4 characters.
 @Test
    fun `first line longer than second line`() {
        val lines = listOf("The fourth line.", "The fifth line.")
        val expected = listOf("TT", "hh", "ee", "  ", "ff", "oi", "uf", "rt", "th", "h ", " l", "li", "in", "ne", "e.", ".")
        assertEquals(expected, Transpose.transpose(lines))
    }

    @Test
    fun `second line longer than first line`() {
        val lines = listOf("The first line.", "The second line.")
        val expected = listOf("TT", "hh", "ee", "  ", "fs", "ie", "rc", "so", "tn", " d", "l ", "il", "ni", "en", ".e", " .")
        assertEquals(expected, Transpose.transpose(lines))
    }

 @Test
    fun `mixed line length`() {
        val lines = listOf("The longest line.", "A long line.", "A longer line.", "A line.")
        val expected = listOf("TAAA", "h   ", "elll", " ooi", "lnnn", "ogge", "n e.", "glr", "ei ", "snl", "tei", " .n", "l e", "i .", "n", "e", ".")
        assertEquals(expected, Transpose.transpose(lines))
    }

andsus avatar Sep 17 '21 02:09 andsus

Just pointing out that I remember finding the same thing obnoxious when I was doing the transpose exercise in various languages. It ends up being a complicated bit about getting the longest line length encountered so far that needs padding rather than a fixed padding for all rows (much easier to do that one). I am not sure the extra challenge of this the way it is written was really all that good as a teaching tool. So, TLDR: I would prefer for it to act like the matrix analogy it mimics as well.

jmrunkle avatar Sep 17 '21 04:09 jmrunkle

The requirement on Readme is very clear that the rows need to be padded for the shorter string, and all the test cases supported that requirement. The only inconsistent is these 2 test cases:

  • first line longer than second line FAILED versu second line longer than first line PASSED
  • mixed line length
TransposeTest > two characters in a column PASSED

TransposeTest > single line PASSED

TransposeTest > two characters in a row PASSED

TransposeTest > second line longer than first line PASSED

TransposeTest > simple PASSED

TransposeTest > square PASSED

TransposeTest > first line longer than second line FAILED
    java.lang.AssertionError: expected:<[TT, hh, ee,   , ff, oi, uf, rt, th, h ,  l, li, in, ne, e., .]> but was:<[TT, hh, ee,   , ff, oi, uf, rt, th, h ,  l, li, in, ne, e., . ]>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at kotlin.test.junit.JUnitAsserter.assertEquals(JUnitSupport.kt:32)
        at kotlin.test.AssertionsKt__AssertionsKt.assertEquals(Assertions.kt:51)
        at kotlin.test.AssertionsKt.assertEquals(Unknown Source)
        at kotlin.test.AssertionsKt__AssertionsKt.assertEquals$default(Assertions.kt:50)
        at kotlin.test.AssertionsKt.assertEquals$default(Unknown Source)
        at TransposeTest.first line longer than second line(TransposeTest.kt:47)

TransposeTest > empty string PASSED

TransposeTest > rectangle PASSED

TransposeTest > mixed line length FAILED
    java.lang.AssertionError: expected:<[TAAA, h   , elll,  ooi, lnnn, ogge, n e., glr, ei , snl, tei,  .n, l e, i ., n, e, .]> but was:<[TAAA, h   , elll,  ooi, lnnn, ogge, n e., glr , ei  , snl , tei ,  .n , l e , i . , n   , e   , .   ]>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at kotlin.test.junit.JUnitAsserter.assertEquals(JUnitSupport.kt:32)
        at kotlin.test.AssertionsKt__AssertionsKt.assertEquals(Assertions.kt:51)
        at kotlin.test.AssertionsKt.assertEquals(Unknown Source)
        at kotlin.test.AssertionsKt__AssertionsKt.assertEquals$default(Assertions.kt:50)
        at kotlin.test.AssertionsKt.assertEquals$default(Unknown Source)
        at TransposeTest.mixed line length(TransposeTest.kt:61)

TransposeTest > triangle PASSED

11 tests completed, 2 failed

My Kotlin solutions:
object Transpose {

    fun transpose(words: List<String>): List<String> {
        if (words.isEmpty()) return emptyList()
        val width = words.map(String::length).max()!!
        val padWords = words.map{ w -> w.padEnd(width) }

        return List(width) { 
            c -> padWords.map{ it[c] }.joinToString("") 
        }
    }
}


andsus avatar Sep 17 '21 06:09 andsus

I disagree with the claim that the test cases are inconsistent, but I believe that this issue can still be used to discuss possible changes to the exercise.

Test cases consistency

The solutions should pad the length to longest length with space, and transpose it

That is very much not what https://github.com/exercism/problem-specifications/blob/main/exercises/transpose/description.md says to do.

If the input has rows of different lengths, this is to be solved as follows:

  • Pad to the left with spaces.
  • Don't pad to the right.

Therefore, the expected output for "first line longer than second line" is consistent with the description.

123456789abcdefg
The fourth line.
The fifth line.

The lines differ in length in column g. In the output, the row corresponding to column g should be "." not ". " because the description says not to pad to the right.

The expected output for "mixed line length" is consistent with the description.

123456789abcdefgh
The longest line.
A long line.
A longer line.
A line.
  • Here, the output rows corresponding to input columns 8 through e inclusive (seven output rows) should all have three characters, not four, because the last line ends at column 7, and we don't pad to the right.
  • We don't disagree that the output rows corresponding to input columns d and e should have their second character be a space, so no further explanation needed here.
  • Output rows corresponding to input columns f through h inclusive (three output rows) should have just one character, since only the first line (which is the longest line) has characters in those columns, and we don't pad to the right.

Possible changes

When writing the above section about consistency, one point stood out where the description is unclear. It says "don't pad to the right", but it didn't tell us whether it means "don't pad the input to the right" or "don't pad the output to the right". It means the latter. I would like to suggest this be changed.

There is also an argument that things could be made considerably easier if the requirements were changed to always pad all lines to the longest line length and therefore the output would always be a perfect rectangle. I dislike this because it means more characters than necessary would be in the output. Currently I believe all expected outputs contain the minimum number of characters necessary to make the transposition correct. However, if it is decided that making that change would better support the teaching objectives of the exercise, I know I shouldn't get in the way of that.

petertseng avatar Sep 20 '21 10:09 petertseng

When writing the above section about consistency, one point stood out where the description is unclear. It says "don't pad to the right", but it didn't tell us whether it means "don't pad the input to the right" or "don't pad the output to the right". It means the latter. I would like to suggest this be changed.

This (changing the description to be more explicit) would also be my preference.

ErikSchierboom avatar Jan 13 '22 15:01 ErikSchierboom