trjhtr

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) 

 private static void printSampleUsageToUser() 
 final String sampleUsageDirective = "Sample usage: java -jar MrPackerUnpacker (" + PackerUnpacker.CHOICE_PACK + "

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.

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.