Loop vs closure, readable vs concise?

Question

In my answer to my own question here I posted some code and @Dave Newton was kind enough to provide me with a gist and show me the error in my not-so-Groovy ways. <-- Groovy pun

I took his advice and revamped my code to be Groovier. Since then the link I am making (which Dave represents with the replaceWith variable) has changed. Now the closure representation of what I want to do would look like this:

int i = 1
errorList = errorLinksFile.readLines().grep { it.contains "href" }.collect { line ->
    def replaceWith = "<a href=\"${rooturl}${build.url}parsed_console/log_content.html#ERROR${i++}\">"
    line.replaceAll(pattern, replaceWith).minus("</font>")
}

And the for loop representation of what I want to do would look like this:

def errorList = []
def i = 1
for(line in errorLinksFile.getText().split("\n")){
    if(!line.contains("href")){
        continue
    }
    errorList.add(line.replaceAll(pattern, "<a href=\"${rooturl}${build.url}parsed_console/log_content.html#ERROR${i++}\">").minus("</font>"))
}

The closure version is definitely more concise, but I'm worried if I always go the "Groovier" route the code might be harder for other programmers to understand than a simple for loop. So when is Groovier better and when should I opt for code that is likely to be understood by all programmers?

Answer 1

I believe that a development team should strive to be the best and coding to the least knowledgeable/experienced developer does not support this. It is important that more than one person on the team knows how to read the code that is developed though. So if you're the only one that can read it, teach someone else. If you're worried about someone new to the team being able to read it I feel that they would be equally hard to read since there would be lack of domain knowledge. What I would do though is break it up a little bit:

def originalMethod() {
    //Do whatever happens before the given code
    errorList = getModifiedErrorsFromFile(errorLinksFile)
}

def getModifiedErrorsFromFile(errorLinksFile) {
    int i = 1
    getHrefsFromFile(errorLinksFile).collect { line ->
        def replaceWith = getReplacementTextForLine(i)
        i++
        line.replaceAll(pattern, replaceWith).minus("</font>")
    }
}

def getHrefsFromFile(errorLinksFile) {
    errorLinksFile.readLines().grep { it.contains "href" }
}

def getReplacementTextForLine(i) {
    "<a href=\"${rooturl}${build.url}parsed_console/log_content.html#ERROR${i}\">"
}

This way if the next person doesn't immediately understand what is going on they should be able to infer what is going on based on the method names. If that doesn't work adding tests would help the next person understand what is going on.

My 2 cents. Good topic though!!!