0
\$\begingroup\$

Write a Compress class defines two static methods, gzipFile(), which compresses a file using GZIP compression format, and zipDirectory(), which compresses the files (but not directories) in a directory using the ZIP archive and compression format. gzipFile() uses the GZIPOutputStream class, while zipDirectory() uses the ZipOutputStream and ZipEntry classes, all from java.util.zip.

I am outputting the compressed file in the same directory as the input file.
For zipDirectory() method I have recursively found out the files present in the directory(including those in the sub directories), stored them in a List<File>, and added them to the zip.
How can I improve this code?

package beg_assignment8;

import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.util.zip.GZIPOutputStream;
import java.util.zip.ZipOutputStream;
import java.util.zip.ZipEntry;

public class Compress {
    public static void main(String[] args) throws FileNotFoundException, IOException {
        Scanner sc = new Scanner(System.in);
        int op = sc.nextInt();
        String source = sc.next();
        switch (op) {
        case 1:
            GZIPFile(source);
            break;
        case 2:
            zipDirectory(source);
            break;
        }
    }

    public static void GZIPFile(String sourcePath) throws FileNotFoundException, IOException {
        File file = new File(sourcePath);
        String destPath = sourcePath + ".gz";


        try (
            FileOutputStream fos = new FileOutputStream(destPath);
            GZIPOutputStream gzos = new GZIPOutputStream(fos)
        ) {
            writeFileToOutputStream(file, gzos);
        }
    }

    public static void zipDirectory(String sourcePath) throws FileNotFoundException, IOException {
        File dir = new File(sourcePath);
        List<File> allFiles = getAllFiles(dir);
        String destPath = sourcePath + ".zip";

        try (
            FileOutputStream fos = new FileOutputStream(destPath);
            ZipOutputStream zos = new ZipOutputStream(fos)
        ) {
            for (File file : allFiles) {
                String zipFile =
                    file.getCanonicalPath().substring(dir.getCanonicalPath()
                            .length() + 1);
                ZipEntry ze = new ZipEntry(zipFile);
                zos.putNextEntry(ze);
                writeFileToOutputStream(file, zos);
                zos.closeEntry();
            }
        }
    }

    private static List<File> getAllFiles(File dir) {
        File[] files = dir.listFiles();
        List<File> allFiles = new ArrayList<>();
        for (File file : files) {
            if (file.isDirectory()) {
                List<File> allFilesRecur = getAllFiles(file);
                allFiles.addAll(allFilesRecur);
            } else {
                allFiles.add(file);
            }
        }
        return allFiles;
    }

    private static void writeFileToOutputStream(File file,
            OutputStream os) throws FileNotFoundException, IOException {
        try (
            FileInputStream fis = new FileInputStream(file);
            BufferedInputStream bis = new BufferedInputStream(fis)
        ) {
            byte[] buffer = new byte[1024];
            int nBytesRead;
            while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
                os.write(buffer);
            } 
        };
    }
}
\$\endgroup\$
3
\$\begingroup\$

nBytesRead ignored!

        int nBytesRead;
        while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
            os.write(buffer);
        }

You forgot to use the nBytesRead variable when writing to the other stream, putting garbage data in the target file.

        int nBytesRead;
        while ((nBytesRead = bis.read(buffer, 0, buffer.length)) >= 0) {
            os.write(buffer, 0, nBytesRead);
        }

zipDirectory() throws a NPE

Your code throws a NullPointerException if the directory doesn't exists. (Caused by dir.listFiles(); returning null, failing the for loop after)

You should check if dir.exists is true, before continuing with the method, and throw a FileNotFoundException if it is false.

Possible IndexOutOfBoundsException

String zipFile = file.getCanonicalPath().substring(dir.getCanonicalPath()
                        .length() + 1);

While this line usually works correctly, it fails on unix systems if the file is symlinked to a directory with less characters, this will throw a IndexOutOfBoundsException

\$\endgroup\$
  • 1
    \$\begingroup\$ Don't check dir.exists (permissions, time races, etc, may still lead to listFiles() failure). Correct way it to check dir.listFiles() return value. \$\endgroup\$ – vnp Mar 23 '16 at 17:12

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.