HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Using Enum to apply Consumer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
enumconsumerusingapply

Problem

I have a utility called ZipFileCombiner that combines zip archives into a single archive. Part of the implementation is using an Enum called CollisionStrategy to deal with entry collisions. It's a nested Enum. It seems kind of ugly to me. It feels like embedding that much code within the Enum seems wrong. I could create yet another inner class (or outer) to hold it, but that seems wrong as well since logic is tightly coupled to the idea of a strategy. I'm looking for suggestions as well as a general peer review.

Full code here in GitHub. (IOUtil is also in that project.)

Relevant pieces:

CollisionStrategy

```
public enum CollisionStrategy {
FAIL((bean, combiner) -> {
throw new IllegalStateException("Collision detected. Entry " + bean.entry() +
" exists. Current source: " + bean.source());
}),
USE_FIRST((bean, combiner) -> {}),
RENAME_AND_ADD((bean, combiner) -> {
if(bean.entry().isDirectory()){
return;
}

String[] sourceNameParts = bean.source().getName().split("/");
String sourceName = sourceNameParts[sourceNameParts.length-1];
String[] entryNameParts = bean.entry().getName().split("/");
String entryName = entryNameParts[entryNameParts.length-1];
entryNameParts[entryNameParts.length-1] = sourceName + entryName;
String newEntryName = Arrays.asList(entryNameParts).stream().collect(Collectors.joining("/"));

// Have to manually "clone" the entry. This sucks.
ZipEntry entry = new ZipEntry(newEntryName);
entry.setTime(bean.entry().getTime());
entry.setComment(bean.entry().getComment());
entry.setCompressedSize(bean.entry().getCompressedSize());
entry.setCrc(bean.entry().getCrc());
entry.setCreationTime(bean.entry().getCreationTime());
entry.setMethod(bean.entry().getMethod());
entry.setExtra(bean.entry().getExtra());
entry.setLastAccessTime(bean.entry().g

Solution

-
It's best practices to let statements like if, for, ... be followed by a space to disinguish them from method invocations.

  • CollisionStrategy



I'd use:

String.format("Collision detected. Entry %s exists. Current source: %s",
    bean.entry(), bean.source());


instead of string concatenation:

"Collision detected. Entry " + bean.entry() +
    " exists.  Current source: " + bean.source()


The same applies to addEntryContent().

  • CombinerBean



Java best practices is "one statement per line" since your code is a lot more read (possibly by others, too) than it is written.

  • addEntryContent() and copyEntryFromSourceToTarget()



You break lines at ~104 in CollisionStrategy but you don`t do it in these methods. That's not consistent style.

Code Snippets

String.format("Collision detected. Entry %s exists. Current source: %s",
    bean.entry(), bean.source());
"Collision detected. Entry " + bean.entry() +
    " exists.  Current source: " + bean.source()

Context

StackExchange Code Review Q#97955, answer score: 2

Revisions (0)

No revisions yet.