2
\$\begingroup\$

I'm compressing an array of strings in an Android app. Is there something you would suggest?

public static String compressArray(String[] data){
    String jsonData = Arrays.toString(data);

    try {
        String compressedData = compressString(jsonData);
        int compressedLength = compressedData.length();
        int normalLength = jsonData.length();
        return normalLength < compressedLength? jsonData:compressedData;
    } catch (IOException e) {
        e.printStackTrace();
        return "";  
    }

}
public static String compressString(String srcTxt) throws IOException {
    ByteArrayOutputStream rstBao = new ByteArrayOutputStream();
    GZIPOutputStream zos = null;
    try {
        zos = new GZIPOutputStream(rstBao);
        zos.write(srcTxt.getBytes());
        zos.finish();

        byte[] bytes = rstBao.toByteArray();
        return Base64.encodeBase64String(bytes);

    }
    finally {
        zos.close();
        rstBao.close();

    }

}
\$\endgroup\$
2
\$\begingroup\$

Compressed vs. non

        return normalLength < compressedLength? jsonData:compressedData;

Isn't this backwards? Consider

        return normalLength <= compressedLength? jsonData : compressedData;

or

        return compressedLength < normalLength ? compressedData : jsonData;

Both alternatives do the same thing. The second version is one character shorter and may be a little clearer about what it is doing. If the compressedLength is less than the normalLength substitute the compressedData for the normal data.

The original returns a compressed version, which has to be uncompressed, when the compressed and the uncompressed data are the same length. This version returns the uncompressed version in that case. Same space, but less time (because it can just use the data rather than decompressing it first).

It's not going to be a big deal either way. Not only is this likely to be rare, it has little impact when it does happen. It just struck me as being off and easy to fix.

What does close actually do?

    GZIPOutputStream zos = null;
    try {
        zos = new GZIPOutputStream(rstBao);
        zos.write(srcTxt.getBytes());
        zos.finish();

        byte[] bytes = rstBao.toByteArray();
        return Base64.encodeBase64String(bytes);

    }
    finally {
        zos.close();
        rstBao.close();

    }

You don't need to close rstBao for two reasons.

First, look at the Javadoc for close:

Writes remaining compressed data to the output stream and closes the underlying stream.

Note the last part. The "underlying stream" in this case is rstBao. So closing zos automatically closes rstBao for you.

Second, if you look at the Javadoc for ByteArrayOutputStream, the close method:

Closing a ByteArrayOutputStream has no effect. The methods in this class can be called after the stream has been closed without generating an IOException.

So it is entirely unnecessary to close rstBao. But you are effectively doing it twice: once implicitly by closing zos and once explicitly.

The smallest change would drop the second close, but a larger change may better use the notions of the try format. Consider

    try (GZIPOutputStream zos = new GZIPOutputStream(rstBao)) {
        zos.write(srcTxt.getBytes());
    }

    byte[] bytes = rstBao.toByteArray();
    return Base64.encodeBase64String(bytes);

This moves the byte array manipulations out of the try block, as they don't throw exceptions nor do they require the compression stream.

Moving them out also means that it's not necessary to call finish explicitly. When the try-with-resources finishes, it will automatically close the the stream and write the data.

This still closes rstBao implicitly, but as discussed it won't do anything. This is how a ByteArrayOutputStream is designed to operate. It ignores the close the format requires and continues.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your thorough answer. 1.- About the return conditions, aren't both of them equivalent? 2.- About discarding a compressed string. In our project there are long strings of ~300k characters and small ones of ~50 characters. However, the shorter ones are twice as frequent so we want to save as much space as possible since the long strings are 90% shorter after compression but the short ones are 50% larger afterward. 3.- About the try block. You are right, thanks a lot. It looks much better. \$\endgroup\$ – Julio Nov 21 '16 at 17:29
0
\$\begingroup\$

The code looks quite good in general, but I'd change to try with resource which closes the Closables automatically:

public static String compressString(String srcTxt) throws IOException {
    try (ByteArrayOutputStream rstBao = new ByteArrayOutputStream();
         GZIPOutputStream zos = new GZIPOutputStream(rstBao);) {
        zos.write(srcTxt.getBytes());
        zos.finish();

        byte[] bytes = rstBao.toByteArray();
        return Base64.encodeBase64String(bytes);

    }
}
| 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.