Thanks for your review Stuart. Comments below.
> On 6 Dec 2018, at 02:18, Stuart Marks <stuart.ma...@oracle.com> wrote: > > Hi Vinnie, > > Roger Riggs wrote: >>> The 'forEachOrdered' should not be necessary and may raise questions about >>> why. >>> if there's no good reason, use 'forEach’. > > Using forEachOrdered() is necessary. The dumpAsStream() method produces a > stream; presumably it has a defined order that's the same as its input. The > semantics of Stream.forEach() are extremely relaxed, and in particular it's > not required to process stream elements in order, even if the stream is > ordered. (In practice this is noticeable if the stream is run in parallel.) > > I'd use forEachOrdered() both in the examples and also where you use it in > implementations. I’ve retained forEachOrdered() throughout. > > The methods that return streams should specify the important characteristics. > Probably the ones most important one are that the returned stream is ordered > and sequential. For the overloads that take fixed-size input, the resulting > stream might also be SIZED. I’ve updated the javadoc @return tag for the Stream-returning methods. Let me know if you’d prefer the stream characteristics to also appear in the method’s first sentence. > > I'm not convinced that the overloads that send output to an OutputStream pull > their weight. They basically wrap the OutputStream in a PrintStream, which > conveniently doesn't declare IOException, making it easy to use from a lambda > passed to forEachOrdered(). If an error writing the output occurs, this is > recorded by the PrintStream wrapper; however, the wrapper is then thrown > away, making it impossible for the caller to check its error status. The intent is to support a trivial convenience method call that generates the well-known hexdump format. Especially for users that are interested in the hexdump data rather than the low-level details of how to terminate a stream. The dumpAsStream methods are available to support cases that differ from that format. Have you a suggestion to improve the dump() methods, or you’d like to see them omitted? > > The PrintStream wrapper also uses the platform default charset, and doesn't > provide any way for the caller to override the charset. Is there a need for that? Originally the requirement was driven by the hexdump format which is ASCII-only. Recently the class has been enhanced to also support the printable characters from ISO 8859-1. A custom formatter be supplied to dumpAsStream() to cater for all other cases? > > Instead, you can just provide the Stream-returning methods, and let the user > send the output to a PrintStream using forEachOrdered() as in your examples. > > It might be nice to provide convenience APIs to send output elsewhere, but > the problem is that it seems difficult to do so without losing control over > things like error handling or charsets. In particular, since the hex > formatter is producing strings, it seems like there should be an option to > send the output to a Writer. Unfortunately it's difficult to do so from a > Stream, because all the Writer methods throw IOException. However, solving > this isn't hexdump's problem. It’s a little more effort but those cases can always be handled by a custom formatter. > > s'marks
