4
\$\begingroup\$

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().getLastAccessTime());
        entry.setLastModifiedTime(bean.entry().getLastModifiedTime());
        try (InputStream in = bean.source().getInputStream(bean.entry())){
            combiner.copyEntryFromSourceToTarget(in,entry,bean.zipOutputStream());
        } catch (IOException e) {
            throw new IllegalStateException(e);
        }
    });

    BiConsumer<CombinerBean,ZipFileCombiner> biConsumer;

    CollisionStrategy(BiConsumer<CombinerBean,ZipFileCombiner> consumer){
        biConsumer = consumer;
    }

    void apply(CombinerBean bean, ZipFileCombiner combiner){
        biConsumer.accept(bean,combiner);
    }
}

CombinerBean

/*
CombinerBean is used in the CollisionStrategy.  It only serves to clean up code such that long arg lists
aren't necessary.
 */
private static class CombinerBean{
    private final ZipEntry e;
    private final ZipFile src;
    private final ZipOutputStream out;

    public CombinerBean(ZipOutputStream zo, ZipEntry ze, ZipFile source){
        e = ze; src = source; out = zo;
    }

    public ZipEntry entry(){ return e; }
    public ZipFile  source(){ return src; }
    public ZipOutputStream zipOutputStream(){ return out; }
}

Method addEntryContent that refers to the strategy

private void addEntryContent(final ZipOutputStream out, final ZipFile source, final ZipEntry entry, final Set<String> entryNames) throws IOException {
    if(!entryNames.add(entry.getName())){
        // Assuming duplicate directory entries across archives are OK, so skip if directory
        if(!entry.isDirectory()) {
            LOGGER.warning(entry.getName() + " has already been added. Applying strategy: " + strategy);
            strategy.apply(new CombinerBean(out, entry, source), this);
        }
    }else {
        try (InputStream in = source.getInputStream(entry)) {
            copyEntryFromSourceToTarget(in, entry, out);
        }
    }
}

Method copyEntryFromSourceToTarget which is called by the strategy

private void copyEntryFromSourceToTarget(final InputStream in, final ZipEntry targetEntry, final ZipOutputStream out) throws IOException {
    out.putNextEntry(targetEntry);
    IOUtil.readWrite(in, out);
    out.closeEntry();
}
\$\endgroup\$
  • \$\begingroup\$ Not an answer but I usually keep enum's in all capital letters or some other way to easily pick out. \$\endgroup\$ – Evan Carslake Jul 24 '15 at 15:45
  • 1
    \$\begingroup\$ An alternative to wrapping a lambda is to make the enum CollisionStrategy implements BiConsumer<CombinerBean,ZipFileCombiner> \$\endgroup\$ – Peter Lawrey Jul 25 '15 at 5:52
2
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I agree with your points in general, but I don't apply blanket rules to my code. If a certain style for a block of code seems easier to read, even if it breaks conventions, then use it. I understand it's a subjective call, so maybe others don't feel the same. \$\endgroup\$ – MadConan Jul 27 '15 at 13:07
  • \$\begingroup\$ @MadConan What are you referring to in detail? \$\endgroup\$ – GeroldBroser reinstates Monica Jul 27 '15 at 13:10
  • \$\begingroup\$ CombinerBean, one statement per line. For the simplistic, one-line-code-blocks in a small inner class, I personally prefer seeing it all together at a glance as opposed to scrolling back and forth. \$\endgroup\$ – MadConan Jul 27 '15 at 13:13
  • \$\begingroup\$ The String.format suggestion is a good one, though. I need to start using that more. Old habits die hard :) \$\endgroup\$ – MadConan Jul 27 '15 at 13:16
  • \$\begingroup\$ @MadConan With 8 more lines you have to scroll? Are you developing on a smartphone in landscape view? :-) \$\endgroup\$ – GeroldBroser reinstates Monica Jul 27 '15 at 13:19

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.