3
\$\begingroup\$

Can I improve performance of below method? Is there anything I need to optimize or any redundancy I need to remove to make it more efficient? It's a simple method used to pad strings by left or right.

public static String padString(String input, boolean leftPad, char padChar, int maxLength) {
    String output = CommonBaseConstants.EMPTY_STRING;
    final int defaultSize = 8;
    String filledStr = null;
    if (CommonBaseConstants.CHAR_ZERO == padChar) {
        filledStr = StringUtils.getZeroes(defaultSize);
    }
    else if (CommonBaseConstants.CHAR_SPACE == padChar) {
        filledStr = StringUtils.getSpaces(defaultSize);
    }

    if (null != filledStr && null != input) {
        if (maxLength > input.length()) {
            StringBuilder value = new StringBuilder();
            int remainder = maxLength % defaultSize;
            if (0 != remainder) {
                value.append(filledStr, 0, remainder);
            }

            int loopSize = (maxLength - remainder) / defaultSize;
            for (int index = 0; index < loopSize; index++) {
                value.append(filledStr);
            }

            output = value.toString();
            if (leftPad) {
                output = output.substring(0, maxLength - input.length()) + input;
            }
            else {
                output = input + output.substring(0, maxLength - input.length());
            }
        }
        else {
            output = input;
        }
    }

    return output;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Without an additional indication of what exactly the code is to accomplish (doc comments welcome), it is difficult to suggest improvements, let alone radically different approaches to whatever problem there is to solve. \$\endgroup\$ – greybeard Jun 25 at 11:02
5
\$\begingroup\$

The API you expose is very weird. Consider the following call:

padString("foo", true, '\t', 5);

The result being "" is not really obvious.

Let's just rewrite this from scratch and then compare it to the existing code.


First we're going to have some requirements:

  • padString should not make the input smaller. The code you present correctly deals with that, albeit only comparatively late in the process.
  • padString should expose left and right padding
  • padString should allow customization of the character used for padding.

With this we can start pulling changes into your code.

The easiest of these to fulfil (incidentally simplifying your code) is the first requirement:

    if (input.length() >= maxLength) {
        return input;
    }

Note how I removed the assignment to output and instead directly returned the value to simplify the control stucture.

The next requirement is to enable leftPadding and rightPadding. The cleanest way to deal with that is to create separate methods for that, avoiding the boolean leftPad argument turning the method into a state automaton.

Instead I suggest you expose leftPad and rightPad. If you must keep using the API you present here, (which I highly doubt), you can implement padString as:

return leftPad ? leftPad(input, padChar, maxLength) : rightPad(input, padChar, maxLength);

Now that we have that sorted we want to deal with the actual implementation.

Consider the following simplification:

int padSize = maxLength - input.length();
// generate an "infinite" stream of padChar, limit it to the padSize and collect as String
String padding = Stream.generate(() -> padChar).limit(padSize).collect(Collectors.joining());
StringBuilder builder = new StringBuilder(padding);
if (leftPad) {
    // padding is supposed to be on the left, so append the input
    builder.append(input);
} else {
    // padding is supposed to be on the right, put input before
    builder.insert(0, input);
}
assert (builder.length() == maxLength);
return builder.toString();

At this point you should notice something that can be further simplified. The StringBuilder is just a glorified string concatenation:

return leftPad ? padding + input : input + padding;

Note that the padding generation can be slightly simplified using Arrays.fill:

char[] paddingChars = new char[padSize];
Arrays.fill(paddingChars, padChar);
String padding = new String(paddingChars);

If you want to enforce some more raw performance you could even go so far as to use System.arraycopy, though I have not done any performance testing. Note also that I'm reasonably sure this does not correctly handle unicode codepoints outside of the Basic Multilingual Plane (BMP)...

    if (input.length() >= maxLength) { return input; }
    char[] result = new char[maxLength];
    int padSize = maxLength - input.length();
    if (leftPad) {
        Arrays.fill(result, 0, padSize, padChar);
        input.getChars(0, input.length(), result, padSize + 1);
    } else {
        input.getChars(0, input.length(), result, 0);
        Arrays.fill(result, input.length(), maxLength - 1, padChar);
    }
    return new String(result);

Sidenote: the existing code also does not correctly handle unicode codepoints outside of the BMP. I assume you were not aware of that because most people are not. The gist of it is: Some "characters" require more than 16 bits to be uniquely identifiable. These supplementary characters are ... weird around length and a handful of other String functions.

\$\endgroup\$
  • \$\begingroup\$ Many thanks for the detailed review and re-vamping of entire method. Really appreciated. Works great! \$\endgroup\$ – Tishy Tash Aug 4 at 13:39

Your Answer

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

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