2
\$\begingroup\$

My little program (called MrPackerUnpacker) takes a full path as an argument and creates a single file that can be unpacked to a folder later to reveal the packed files.

Given the following 2 files in folder: /Users/koraytugay/Pictures:

a.txt
koray.txt

where contents of the files are just "a" and "koray" saved in UTF-8 encoding, the created file will have the name Pictures.pckr and the contents:

0000 0000 0000 0005 0000 0000 0000 0001
612e 7478 7461 0000 0000 0000 0009 0000
0000 0000 0005 6b6f 7261 792e 7478 746b
6f72 6179 

where

0000 0000 0000 0005

represents 5 bytes, length of the name of the first file packed.

0000 0000 0000 0001

represents 1 byte, length of the contents of the first file packed.

612e 7478 74

represents the filename of the first file packed. (a.txt in this case).

61

represents the contents of the first file. (simply a in this case).

I think you get the idea how I modeled the "packed file". I have not yet implemented unpacking a packed file, I would highly appricate if you can review the code for my packing a folder:

MrPackerUnpacker.java

import biz.tugay.mrpackerunpacker.PackerUnpacker;

import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;

public class MrPackerUnpacker {

    public static void main(String[] args) {

        // Make sure the user is calling the application the right way.
        // Sample: java -jar MrPackerUnpacker pack /Users/koraytugay/Pictures
        if (args == null || args.length != 2) {
            printSampleUsageToUser();
            return;
        }

        // First argument must be a valid PackerUnpacker choice!
        final String packUnpackUserInput = args[0];
        if (!packUnpackUserInput.equals(PackerUnpacker.CHOICE_PACK)
                && !packUnpackUserInput.equals(PackerUnpacker.CHOICE_UNPACK)) {
            printSampleUsageToUser();
            return;
        }

        final Path path = Paths.get(args[1]);

        final PackerUnpacker packerUnpacker = new PackerUnpacker(packUnpackUserInput, path);
        try {
            packerUnpacker.packUnpack();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    private static void printSampleUsageToUser() {
        final String sampleUsageDirective = "Sample usage: java -jar MrPackerUnpacker (" + PackerUnpacker.CHOICE_PACK + "|" + PackerUnpacker.CHOICE_UNPACK + ") $fullPath ";
        System.out.println(sampleUsageDirective);
    }
}

PackerUnpacker.java

package biz.tugay.mrpackerunpacker;

import biz.tugay.mrpackerunpacker.pack.Packer;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;

public class PackerUnpacker {

    public static final String CHOICE_PACK = "pack";
    public static final String CHOICE_UNPACK = "unpack";
    public static final String EXTENSION = ".pckr";

    private final String packUnpackUserInput;
    private final Path path;

    public PackerUnpacker(final String packUnpackUserInput, final Path path) {
        this.packUnpackUserInput = packUnpackUserInput;
        this.path = path;
    }

    public void packUnpack() throws IOException {
        if (!Files.exists(path)) {
            throw new FileNotFoundException("The path you are trying to pack/unpack does not seem to exist!");
        }

        if (packUnpackUserInput.equals(CHOICE_PACK)) {
            packPath();
        }

        if (packUnpackUserInput.equals(CHOICE_UNPACK)) {
            unpackPath();
        }
    }

    private void packPath() throws IOException {
        final Packer packer = new Packer(path);
        packer.pack();
    }

    private void unpackPath() {
        // Not implemented yet...
    }

}

Packer.java

package biz.tugay.mrpackerunpacker.pack;

import biz.tugay.mrpackerunpacker.PackerUnpacker;
import org.apache.commons.io.IOUtils;

import java.io.*;
import java.nio.file.Files;
import java.nio.file.Path;

public class Packer {

    private final Path pathToPack;

    public Packer(final Path pathToPack) throws IOException {
        final boolean isPathToPackDirectory = Files.isDirectory(pathToPack);
        if (!isPathToPackDirectory) {
            throw new IOException("If you are trying to pack a path, it must be a directory!");
        }
        this.pathToPack = pathToPack;
    }

    public void pack() throws IOException {
        final PackFile packFile = new PackFile(pathToPack);

        final File output = new File(pathToPack.getFileName().toString() + PackerUnpacker.EXTENSION);

        final DataOutputStream dataOutputStream = new DataOutputStream(new FileOutputStream(output));

        for (PackedFileMeta packedFileMeta : packFile.getPackFileMetas()) {
            dataOutputStream.writeLong(packedFileMeta.getFilenameLength());
            dataOutputStream.writeLong(packedFileMeta.getFileLength());
            dataOutputStream.write(packedFileMeta.getFileNameUTF8Encoded());
            IOUtils.copyLarge(new FileInputStream(packedFileMeta.getFilePath().toFile()), dataOutputStream);
        }

        dataOutputStream.flush();
        dataOutputStream.close();
    }
}

PackFile.java

package biz.tugay.mrpackerunpacker.pack;

import java.io.IOException;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;

public class PackFile {

    private List<PackedFileMeta> packedFileMetas = new ArrayList<>();

    public PackFile(final Path pathToPack) throws IOException {
        DirectoryStream<Path> pathToPackDirectoryStream = Files.newDirectoryStream(pathToPack);
        for (Path path : pathToPackDirectoryStream) {
            if (Files.isDirectory(path)) {
                continue; // We do not recursivly pack folders, only files in a folder.
            }
            if (path.getFileName().toString().startsWith(".")) {
                continue; // Skip hidden files!
            }
            final PackedFileMeta packedFileMeta = new PackedFileMeta(path);
            packedFileMetas.add(packedFileMeta);
        }
    }

    public List<PackedFileMeta> getPackFileMetas() {
        return packedFileMetas;
    }
}

PackedFileMeta.java

package biz.tugay.mrpackerunpacker.pack;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;

public class PackedFileMeta {

    private final Path filePath;
    private long filenameLength;
    private long fileLength;
    private byte[] fileNameUTF8Encoded;

    public PackedFileMeta(Path filePath) throws IOException {
        this.filePath = filePath;
        fileNameUTF8Encoded = filePath.getFileName().toString().getBytes(StandardCharsets.UTF_8);
        filenameLength = fileNameUTF8Encoded.length;
        fileLength = Files.size(filePath);
    }

    public Path getFilePath() {
        return filePath;
    }

    public long getFilenameLength() {
        return filenameLength;
    }

    public byte[] getFileNameUTF8Encoded() {
        return fileNameUTF8Encoded;
    }

    public long getFileLength() {
        return fileLength;
    }
}

I am more interested in the readability of my code.

\$\endgroup\$
1
\$\begingroup\$

First of all, the readability of this code is OK. Entities are quite well structured and an effort to respect the SRP principle is seen.

General Design

However, I would suggest a few renaming and design changes. The PackerUpacker class resembles a factory, because it instantiates the business logic entities (Packer or Unpacker). But also it chooses which one to use and triggers the main action: this is too much for it, so let's separate the roles.

We can introduce an interface that provides access to the main action:

public interface PathProcessor {

  void processPath(Path pathToProcess) throws IOException;

}

It may also be seen as an abstract class, because both the implementors might share some similarities (ex. path validation).

PathProcessor will be implemented/extended by both Packer or Unpacker classes.

PathProcessorFactory (former PackerUnpacker) will decide, depending on the user's input arg, which entity to instantiate:

public static PathProcessor newPathProcessor(String userChoice) {
  // instantiate and return `Packer` or `Unpacker` depending on the arg
}

The instructions in the main method now become:

PathProcessor pathProcessor = PathProcessorFactory.newPathProcessor(packUnpackUserInput);
pathProcessor.processPath(path);

Other

Packer

The streams inside pack() are not wrapped in a try-with-resources, but should be, to prevent from leaving an unclosed stream in case of exception.

PackFile

The idea to separate it into a dedicated class is not clear. (BTW, it's not a good practice to include much logic into constuctors, neither to throw checked exceptions from them). Its only role is to wrap packedFileMetas, so why not producing them in a utility method, ex. public static List<PackedFileMeta> collectFilesToPack(Path folderToPack)?

And there should be a try-with-resources wrapping the initialization of pathToPackDirectoryStream.

To check whether a file is hidden or not, path.getFileName().toString().startsWith(".") is not reliable, because a file name may begin with a dot without the file being hidden. A better way to check is Files.isHidden(Path).

PackedFileMeta

All the fields should be final.

Since this object is instantiated only in PackFile collecting sequence and is placed in the same package, I'd suggest to reduce its visibility (and of its methods also) to package level, but this is a very minor issue.

\$\endgroup\$
  • \$\begingroup\$ Thank your for your valuable response. Actually, I forgot to turn out to encapsulate the filename in PackFile. \$\endgroup\$ – Koray Tugay Jan 28 '18 at 4:54

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.