So the question is giving a BIG string, break it up, find the palindromes and then find the shortest length within those sets of palindromes. Here's the code

Main Function

fun main(){
    val bigArray = "Simple, given a string of words, return the length of acdca the "  
            "shortest valav words String will never be empty and you do not need dad to account for different data types."

    println(leastP(bigArray))
}

The Custom Function

fun leastP(s: String): Int {
    val sSplit = listOf(s.split(""))
    val newArray = listOf<String>()

    for (i in sSplit){
        for (j in i.indices){
            if (isPalindrome3(i[j])) newArray.plus(j)
        }
    }

    return newArray.minOf { it.length }


}

    private fun isPalindrome3(s: String): Boolean {
        var i = 0
        var j = s.length -1
        while (i < j){
            if (s[i  ].lowercaseChar() != s[j--].lowercaseChar()) return false
        }
        return true
    }

} 

I get this error

Not sure whats going on or where I messed up. Any help is appreciated.

CodePudding user response：

Your newArray is a read-only list. When you call plus on it, the function does not modify the original list (after all, it is read-only). The List.plus() function returns a new list, which you are promptly discarding by not assigning it to any variable or property.

Then it crashes because it is unsafe to call minOf on an empty list.

Two different ways to fix this:

1. Make the newArray variable a var and replace newArray.plus(j) with newArray = j. The = operator, when used on a read-only list that is assigned to a mutable var variable, calls plus() on it and assigns the result back to the variable.

2. Initialize newArray as a MutableList using mutableListOf() and replace newArray.plus(j) with newArray = j. The = operator, when used with a MutableList, calls add() or addAll() on the MutableList, so it directly changes the original instance.

I didn’t check any of your logic. I’m only answering the question about why it’s crashing.

But as Gidds points out, the logic can be simplified a ton to achieve the same thing you’re trying to do using functions like filter(). A few odd things you’re doing:

• Putting the result ofstring.split("") in a list for no reason
• Using "" to split your string so it’s just a list of one-character Strings instead of a list of words. And you’re ignoring punctuation.
• Filling newArray with indices so minOf will simply give you the first index that corresponded with being a palindrome, so it will always be 0.

Here’s how I might write this function (didn’t test it):

fun leastP(s: String): Int {
    return s.split(" ")
        .map { it.filter { c -> c.isLetter() } }
        .filter { isPalindrome3(it) }
        .minOfOrNull { it.length } ?: 0
}

CodePudding user response：

In addition to the array problem identified in Tenfour04's answer, the code has an additional problem:

split("") splits the string into individual characters, not just individual words. 

If you debug it, you'll find that isPalindrome3() is being called first on an empty string, then on "S", then on "i", and so on.

That's because the empty string "" matches at every point in the input.

The easiest fix is to call split(" "), which will split it at space characters.

However, that might not do exactly what you want, for several reasons: it will include empty strings if the input has runs of multiple spaces; it won't split at other white space characters such as tabs, newlines, non-breaking spaces, en spaces, etc.; and it will include punctuation such as commas and full stops. Splitting to give only words is harder, but you might try something like split(Regex("\\W") to include only letters, digits, and/or underscores. (You'll probably want something more sophisticated to include hyphens and apostrophes, and ensure that accented letters etc. are included.)

---

There's a further issue that may or may not be a problem: you don't specify a minimum length for your palindromes, and so words like a match. (As do empty strings, if the split produces any.) If you don't want the result to be 0 or 1, then you'll also have to exclude those.

Also, the code is currently case-sensitive: it would not count "Abba" as a palindrome, because the first A is in upper case but the last a isn't. If you wanted to check case-insensitively, you'd have to handle that.

---

As mentioned in a comment, this is the sort of thing that should be easy to test and debug. Short, self-contained functions with no external dependencies are pretty easy to write unit tests for. For example:

@Test fun testIsPalindrome3() {
    // These should all count as palindromes:
    for (s in listOf("abcba", "abba", "a", "", "DDDDDD"))
        assertTrue(isPalindrome3(s))
    // But these shouldn't:
    for (s in listOf("abcbb", "Abba", "a,", "abcdba"))
        assertFalse(isPalindrome3(s))
}

A test like that should give you a lot of confidence that the code actually works. (Especially because I've tried to include corner cases that would spot all the ways it could fail.) And it's worth keeping unit tests around once written, as they can verify that the code doesn't get broken by future changes.

And if the test shows that the code doesn't work, then you have to debug it! There are many approaches, but I've found printing out intermediate values (whether using a logging framework or simply println() calls) to be the simplest and most flexible.

---

And for reference, all this can be rewritten much more simply:

fun String.leastP() = split(Regex("\\W"))
                     .filter{ it.length >= 2 && it.isPalindrome() }
                     .minOfOrNull{ it.length }

private fun String.isPalindrome() = this == reversed()

Here both functions are extension functions on String, which makes them a bit simpler to write and to call. I've added a restriction to 2 characters. And if the input is empty, minOfOrNull() returns null instead of throwing a NoSuchElementException.

That version of isPalindrome() isn't quite as efficient as yours, because it creates a new temporary String each time it's called. In most programs, the greater simplicity will win out, but it's worth bearing in mind. Here's one that's longer but as efficient as in the question:

private fun String.isPalindrome()
    = (0 until length / 2).all{ i -> this[i] == this[length - i - 1]}