9
\$\begingroup\$

I need to run a evaluation for large (1 Million) number of components and build a string with error which has comma separated error codes. As the final string is to be persisted in RDBMS TABLE.COLUMN with MAX length for 255, I need to truncate the string (not abruptly) and append an ellipses to indicate there were more failures.

Here is what I have

public class ErrorCodeStringBuilder {

    private boolean isFirst = true;
    private boolean isFull = false;
    private StringBuilder stringBuilder;
    private int MAX_CAP = 250;
    private int curLength = 0;
    private String ELLIPSES = "...";

    public ErrorCodeStringBuilder() {
        stringBuilder = new StringBuilder(20);
    }

    public void append(String str) {
        curLength = curLength + str.length();
        if (curLength >= MAX_CAP) {
            isFull = true;
        }
        checkAndAppend(str);
    }

    private void checkAndAppend(String str) {
        if (!isFull) {
            if (isFirst) {
                stringBuilder.append(str);
                isFirst = false;
            } else {
                stringBuilder.append(",");
                stringBuilder.append(str);
            }
        }
    }

    public String toString() {
        if (isFull) {
            return stringBuilder.toString() + "," + ELLIPSES;
        }
        return stringBuilder.toString();
    }
}

Unit Tests:

import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class ErrorCodeStringBuilderTest {

    @Test
    public void testMaxLength() {
        ErrorCodeStringBuilder errorCodeStringBuilder = new ErrorCodeStringBuilder();
        //10
        errorCodeStringBuilder.append("0123456789");
        //20
        errorCodeStringBuilder.append("0123456789");
        //30
        errorCodeStringBuilder.append("0123456789");
        //40
        errorCodeStringBuilder.append("0123456789");
        //50
        errorCodeStringBuilder.append("0123456789");
        //100
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //150
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //200
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //250
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //260
        errorCodeStringBuilder.append("0123456789");


        String result = errorCodeStringBuilder.toString();
        assertTrue(result, result.length() < 255);
        assertTrue(result.endsWith("..."));
    }


    @Test
    public void testAppend() {
        ErrorCodeStringBuilder errorCodeStringBuilder = new ErrorCodeStringBuilder();
        //10
        errorCodeStringBuilder.append("0123456789");
        //20
        errorCodeStringBuilder.append("0123456789");
        String result = errorCodeStringBuilder.toString();
        assertEquals(result, "0123456789,0123456789");
    }

    @Test
    public void testNotAbruptTruncate() {
        ErrorCodeStringBuilder errorCodeStringBuilder = new ErrorCodeStringBuilder();
        //10
        errorCodeStringBuilder.append("0123456789");
        //20
        errorCodeStringBuilder.append("0123456789");
        //30
        errorCodeStringBuilder.append("0123456789");
        //40
        errorCodeStringBuilder.append("0123456789");
        //50
        errorCodeStringBuilder.append("0123456789");
        //100
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //150
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //200
        errorCodeStringBuilder.append("abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO");
        //254
        errorCodeStringBuilder.append("XXabcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNOXX");


        String result = errorCodeStringBuilder.toString();

        assertEquals(result, "0123456789,0123456789,0123456789,0123456789,0123456789,abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO," +
                "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO" +
                ",abcdefghijklmnopqrstuvwxyz0123456789ABCDEFHIJKLMNO,...");
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ To make the question more complete, you should add the unit tests that you have written to the question. This would demonstrate how the code is used and which edge cases you already considered. \$\endgroup\$ – Roland Illig Feb 25 at 6:49
  • \$\begingroup\$ @RolandIllig Thanks for your time. I updated the questions with the tests. \$\endgroup\$ – dRv Feb 25 at 8:17
7
\$\begingroup\$

Not taking the comma and the ellipses into account when calculating the resulting string length and instead setting the max length to 250 is an ugly hack. The code is also a bit buggy, because if the first error message happens to be 250 characters long, you only get ellipses in the result even though there would have been room for the error message, a comma and ellipses.

You should create a method for calculating the result length if a string was appended.

private int calculateResultingLength(String str) {
    int result = stringBuilder.length();
    if (result > 0) {
        result += 1; // Account for a comma.
    }
    if (errorsDropped) {
        result += ELLIPSES.length();
    }
    result += str.length();

    return result;
}

The second isFull check in checkAndAppend is redundant because you already do that check in the append method and checkAndAppend is private. The isFull is now also misleading, because it tells that an error was dropped. The next error might be shorter and fit into the string. I rename it to errorsDropped.

public void append(String str) {
    if (calculateResultingLength(str) >= MAX_CAP) {
        errorsDropped = true;
    } else {
        performAppend(str);
    }
}

The isFirst field is redundant. You know the append is the first one if the stringBuilder is empty:

private void performAppend(String str) {
    if (stringBuilder.length() > 0) {
        stringBuilder.append(",");
    }
    stringBuilder.append(str);
}

The performAppend method became a bit pointless now. You could just write:

public void append(String str) {
    if (calculateResultingLength(str) >= MAX_CAP) {
        errorsDropped = true;
        return;
    }

    if (stringBuilder.length() > 0) {
        stringBuilder.append(",");
    }
    stringBuilder.append(str);
}

The MAX_CAP and ELLIPSES are named as if they were constants but they are variables. They should be static and final. Also, no need to abbreviate here.

private static final int MAX_CAPACITY = 255;
private static final String ELLIPSES = "...";
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks. I completely missed the comma part. Regarding the first error message being long, I did not consider that case because of the assumption that error codes would be much smaller than that. I agree with all the other comments. \$\endgroup\$ – dRv Feb 25 at 8:50
  • 1
    \$\begingroup\$ Firstly, if you append a 200 character string, then a 100 character string (overflow flagged), then a 40 character string (hey, this one fits!), you get "first-string,third-string,..." where as the OP would return just "first-string,...", so are getting a very different behaviour. Secondly, if you add five 50 character strings, yielding a 254 character temporary result, and then add one more string, the overflow flag is set. When the result is finally requested, 4 additional characters are added to the buffered 254 character string, which exceeds the 255 character limit. \$\endgroup\$ – AJNeufeld Feb 25 at 21:22
  • \$\begingroup\$ I should have mentioned that this was an intentional breach from the original. Since we're talking about displaying errors from a million checks in 255 characters, it seemed like this was mostly "nice to know" and the actual content of the string was pretty irrelevant. Then again... if presented with a task like this in real life I'd question the sensibility of the task and suggest that a simple boolean "there_were_errors" flag was set to the database. \$\endgroup\$ – TorbenPutkonen Feb 26 at 11:17
5
\$\begingroup\$

Test naming

Consider dropping 'test' from the front of your test names and using the extra space to add something describing the expected outcome for the test, maybe something like...

append_overflowMaxLength_maxLengthNotExceeded
append_withinBuffer_addsMessage
append_overflowMaxLength_entireExceptionReplacedWithElipses

assertEquals

You're passing your parameters to assertEquals the wrong way round (your expected is your actual). Frameworks like assertJ can make assertions more intuitive.

assertThat(result).isEqualTo("...");
assertThat(result).endsWith("...");
assertThat(result).hasSizeLessThan(255);
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ These names are still a bit word soup. I prefer as plain english as possible. Like "appendsEllipsesWhenMessageIsDropped()" or "separatesMessagesWithComma()". \$\endgroup\$ – TorbenPutkonen Feb 25 at 12:12
  • 1
    \$\begingroup\$ @TorbenPutkonen that's fair enough, naming's somewhat stylistic and it's not uncommon to have some indication of the method under test in the name of the test. If you prefer as plain English as possible, then I think you would prefer the fluent assertions since they tend to read that way. \$\endgroup\$ – forsvarir Feb 25 at 12:21
  • \$\begingroup\$ For classes that have many public methods that is a well justified practise. \$\endgroup\$ – TorbenPutkonen Feb 25 at 13:24
3
\$\begingroup\$

Torben Putkonen's answer is good, but it still has some flaws.

If stringBuilder.length() == 199, and you add message with string length 53, then the string length with become 199 + 1 + 53 == 253 < MAX_CAPACITY. Now you have a problem. If you add another string, say length 50, you exceed the capacity and set errorsDropped = true;.

Now, when toString() is called, the stringBuilder().toString() + ",..." is returned, which is 257 characters! This is too big for the "RDBMS TABLE.COLUMN with MAX length of 255".

There are two approaches you can take:

  1. Limit the accumulation length to 251 characters (255 less the 4 characters in ",...")
  2. Allow the accumulation length to reach 255 characters, but remove the last message(s) if insufficient room exists to add ",..." when necessary.

Demonstrating approach #2, incorporating Torben's private static final members & removal of redundant fields. Note: Assumes the error messages themselves have no embedded commas.

public class ErrorCodeStringBuilder {

    private static final int MAX_CAPACITY = 255;
    private static final String ELLIPSES = "...";

    private StringBuilder stringBuilder = new StringBuilder(20);
    private boolean overflow = false;

    public void append(String str) {
        if (!overflow) {
            if (stringBuilder.length() > 0)
                stringBuilder.append(',');

            stringBuilder.append(str);

            int length = stringBuilder.length();
            if (length > MAX_CAPACITY) {
                int last_comma = stringBuilder.lastIndexOf(",", MAX_CAPACITY - ELLIPSES.length());
                stringBuilder.replace(last_comma + 1, length, ELLIPSES);
                overflow = true;
            }
        }
    }

    @Override
    public String toString() {
       return stringBuilder.toString();
    }
}

Using .lastIndexOf(",", MAX_CAPACITY - ELLIPSES.length()) ensures we find a comma at position 252 or earlier. If the second last message ends at 253, and another message is added with a comma at position 254, an earlier comma will be found, and the these last two messages are replaced with the ellipses.

Bonus: This even handles the edge case of the first string longer than 252 characters. If the first string is 300 characters, then last_comma will become -1 (not found), and .replace(last_comma + 1, length, ELLIPSES) will replace starting from index 0 ... which is the entire string.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ thanks for pointing this out. I added a unit test for this case. \$\endgroup\$ – dRv Feb 26 at 9:37
1
\$\begingroup\$

I think my approach to this problem would be to separate it into two completely independent classes.

The first class would take a List<String> and modify that collection to conform to your length constraint. The result of this transformation could be a new, potentially shorter, List<String>, perhaps with a "..." as the last element where required. Or it could be an object that encapsulates an int for the number of entries to keep and a boolean for whether truncation was necessary.

The second class would then create the comma-separated list. The implementation of this will depend on how you implemented the first class. Don't forget that if you're using Java 8 then you can use the following quite delightful snippet of code to turn a List<String> into a comma-separated String:

list.stream().collect(Collectors.joining(","))
|improve this answer|||||
\$\endgroup\$

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.