current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I wrote the following groovy function for taking a string of multiple lines (or any other character for splitting the code into parts), and bringing each line/part to a specified number of characters, cutting too long lines and filling up too short lines with a fill character:

static final String adjustLineLength(Integer length, String filler, String token, String source) {
    source
    .tokenize(token)
    .collect{ String line ->
        if(line.size() > length)
            line.substring(0, length)
        else if(line.size() < length)
            line + (filler * (length - line.size()))
        else
            line
    }
    .join(token)
}

Is that a good approach? Any suggestions how this code can be improved in concerns of making it more easy to understand and maybe (but that has not priority over beeing easy to understand) improving performance?

share|improve this question

1 Answer 1

up vote 3 down vote accepted

I think your approach is fine, and it's easy to understand. I have just a few improvement ideas:

  • A more natural ordering of the method parameters might be: source, token, length, filler
  • Instead of Integer, int should be enough and shorter
  • Move the closure in collect to its own method
  • Do you really need the static final String declaration? Why not use simply def?
  • Do you really need the type declarations in the signature? You could omit them, and it will be still quite clear

  • The brackets around the multiplication are unnecessary in

    line + (filler * (length - line.size()))

Suggested implementation

Putting the above suggestions together:

def adjustLineLength = { line, length, filler ->
    if (line.size() > length) {
        line.substring(0, length)
    } else if (line.size() < length) {
        line + filler * (length - line.size())
    } else {
        line
    }
}

def adjustTextLength = { source, token, length, filler ->
    source
    .tokenize(token)
    .collect { adjustLineLength(it, length, filler) }
    .join(token)
}
share|improve this answer
    
Hi, the reason why I used this "odd" ordering is because of currying. I want to use curry and if I order it like you posted (what I would do when not having to curry it) then I have to use ncurry and that is a little ugly. Any suggestions what to do in this case? Regarding the "verbose" syntax: I want it to be more familiar for java developers. But thx for the other points, they are really helpfull for me. =) –  valenterry May 8 '14 at 20:15
    
I don't actually get your point about currying, but that doesn't mean anything, because I don't know groovy well at all (and so I cannot comment about the ugliness and ncurry). As for making it more java-developer-friendly, I would boldly go and embrace groovy, but that may be subjective ;-) –  janos♦ May 8 '14 at 21:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.