On 2014-12-29, Kristian Rosenvold wrote: > The refactoring os ZipArchiveOutputStream to use StreamCompressor is now done > in > the branch https://github.com/krosenvold/commons-compress
Some code comments: * the fields writtenToOutputStream, sourcePayloadLength and actualCrc in StreamCompressor can probably be made private * inside the streams we usually write to the underlying stream first and update the count later - so the count is never bigger than what has been written if the write throws an exception. I don't think this also applied to StreamCompressor's writeCounted but may be nice for consistency. * ZipArchiveOutputStream's Deflater has been protected and is now removed. We'll need to discuss whether we all can accept the potentially breaking change. > As refactorings come it doesn't "feel" too good (extracting all the > methods to create zip headers to a different class would probably be a > refactoring with a lot better *feeling* to it....). This may be due to > the overall largeness of the file in question, it may also be because > of the "leaks" between ZipArchiveOutputStream (fields raf & out) and > StreamCompressor. You could probably remove the out field if you added a flush method to the stream compressor. But the raf field remains for rewriting sizes in seekable output - I don't see how this could be moved to StreamCompressor without feeling strange. I'd like to see the OutputStream and RandomAccessFile versions of StreamCompressor#create go away in favour of BackingStore implementations, this could remove the inner subclasses at the same time. The RandomAccessFile case could probably be merged into FileBasedBackingStore where the BackingStore preferred RandomAccessFile when available and falls back to FileOutputStream. Unfortunately that would just duplicate the logic from ZipArchiveOutputStream since that one still needed access to the RandomAccessFile. Yes, I can see how splitting out the code that writes the metadata parts of the archive from those that write actual file data might help. The later part could again be separated for DEFLATED and STORED. What I don't see is how to do it in a backwards compatible manner. Stefan --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
