Compressing a file using zip or gzip

Question

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);
 } 
 };
 }
}

Answer 1

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

• vnp: Don't check dir.exists (permissions, time races, etc, may still lead to listFiles() failure). Correct way it to check dir.listFiles() return value.