2014-12-29 15:05 GMT+01:00 Stefan Bodewig <bode...@apache.org>: > 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. I fixed all comments and reinstated the protected Deflater. > >> 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 think the only way to make this good is to avoid putting all the seek stuff into the streamcompressor; that's what I did. The whole ZipArchiveOutputStream class reminds me of a few of the 3000+ LOC java classes I refactored in maven core; sometimes the only acceptable solution is to extract *all* the logic into other classes and just make the "old" class keep instantiate all the fields and pass them on to the underlying "new" classes with cleaner responsibilities. I'm not really saying this should be done, but the class has that "distinct" feeling :) I took Gary's "cheap" solution to the StreamCompressor "subclass" issue; I just made the whole streamcompressor package private and not a part of any public api. Given that it's not really meant to be an extension point I think this is adequate. Alternately I would actually consider going back to the old "if" statement that used to be there before I introduced all the OO political correctness. Kristian --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
