Conversation
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Some note. I'll comment in the ticket on a separate issue.
| * @param trailerLength the length of the trailer which is hold back (must be >= 0). | ||
| * @throws IOException initializing the trailer buffer failed. | ||
| */ | ||
| public TrailerInputStream(final InputStream source, final int trailerLength) |
There was a problem hiding this comment.
Use the Builder pattern to avoid constructor creep. For example org.apache.commons.io.input.BOMInputStream.Builder.
There was a problem hiding this comment.
What should be the default value of trailerLength? 0 is rather useless, so both arguments have to be provided anyway.
There was a problem hiding this comment.
Personally I prefer constructors, especially when as here there's exactly one constructor.
There was a problem hiding this comment.
There does not need to be a default if none makes sense, an exception can be thrown at build time or in the private ctor for nonsensical values.
If experience has taught us anything here, it's that there will be constructor creep in the future, so please use a builder.
There was a problem hiding this comment.
IMHO builders are vastly overused. They're not native to Java's design, and constructors are more natural. There's a place for builders, but they shouldn't be the default.
| * @param trailerLength the length of the trailer which is hold back (must be >= 0). | ||
| * @throws IOException initializing the trailer buffer failed. | ||
| */ | ||
| public TrailerInputStream(final InputStream source, final int trailerLength) |
There was a problem hiding this comment.
Personally I prefer constructors, especially when as here there's exactly one constructor.
|
|
||
| @Override | ||
| public int available() throws IOException { | ||
| return this.source.available(); |
There was a problem hiding this comment.
shouldn't this subtract the bytes in the trailer?
There was a problem hiding this comment.
The trailer is filled in the constructor. So every single byte read after the constructor finished causes exactly one byte to return to the caller of the read methods. Think about it as shift register.
ax-lothas
force-pushed
the
IO-427_TrailerInputStream
branch
from
2261f2b to
59d74a3
Compare
sebbASF
left a comment
sebbASF
left a comment
There was a problem hiding this comment.
Please update code with design info
|
If this class is indeed generally useful, can it be reused from our Tailer classes? |
| */ | ||
| public TrailerInputStream(final InputStream source, final int trailerLength) | ||
| throws IOException { | ||
| if (trailerLength < 0) { |
There was a problem hiding this comment.
I don't see the point of allowing a zero-length trailer
Also, it probably make sense to have an upper limit.
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Getting the trailer before the stream is fully read should probably throw an illegal state exception.
| * | ||
| * <p> | ||
| * "Normal" read calls read the underlying stream except the last few bytes (the trailer). The | ||
| * trailer is updated with each read call. The trailer can be gotten by one of the copyTrailer |
| * </p> | ||
| * | ||
| * <p> | ||
| * It is safe to fetch the trailer at any time but the trailer will change with each read call |
There was a problem hiding this comment.
That seems wonky. I'd almost prefer iot to throw IllegalStateException unitl it has the real trailer
|
|
||
| private final InputStream source; | ||
| /** | ||
| * Invariant: After every method call which exited without exception, the trailer has to be |
| * </p> | ||
| * | ||
| * @param source underlying stream from which is read. | ||
| * @param trailerLength the length of the trailer which is hold back (must be >= 0). |
| * @param trailerLength the length of the trailer which is hold back (must be >= 0). | ||
| * @throws IOException initializing the trailer buffer failed. | ||
| */ | ||
| public TrailerInputStream(final InputStream source, final int trailerLength) |
There was a problem hiding this comment.
IMHO builders are vastly overused. They're not native to Java's design, and constructors are more natural. There's a place for builders, but they shouldn't be the default.
| return this.trailer.length; | ||
| } | ||
|
|
||
| public byte[] copyTrailer() { |
|
Whilst the class seems to work well, I'm not convinced that there is a sufficient need for it. Every extra class adds to the maintenance load, so there needs to be a demonstrated to make it worth the extra effort. |
Right, I agree, and as I mentioned earlier: Could this be reused as is within IO itself (Tailer class) and from Tika which provides a similar class? |
4 participants
Add a new TrailerInputStream which holds back the last x bytes of the underlying input stream while reading it.
This is useful, e.g., when a payload is followed by a fixed-length checksum.
https://issues.apache.org/jira/browse/IO-427