I was wondering if the TBufferedFileStream (see https://delphiaball.co.uk/2016/04/29/faster-filestream-tbufferedfilestream/) would not have been implemented better by using the decorator pattern (see https://github.com/project-jedi/jcl/blob/master/jcl/source/common/JclStreams.pas#L207) instead of hardcoding it on TFileStream.
I was wondering if the TBufferedFileStream (see https://delphiaball.co.uk/2016/04/29/faster-filestream-tbufferedfilestream/) would not have been implemented better by using the decorator pattern (see https://github.com/project-jedi/jcl/blob/master/jcl/source/common/JclStreams.pas#L207) instead of hardcoding it on TFileStream.
Any opinions?
Any opinions?
Indeed it would.
ReplyDeleteInteresting idea. Put in a feature request, maybe that can be changed in the future.
ReplyDeleteOf course, it should have been, since it would have benefit to any TStream, e.g. over communication or encryption streams...
ReplyDeleteI can understand why they didn't. FileStream covers lots of use cases and Indy does have buffer support for your internet. That's 90% covered. However the decorator would have been a great addition. Emba needs to do much more architectural thinking and get out of this fix a small problem here and there mode.
ReplyDeleteAnother option would have been to add abstract buffer support to TStream itself and then allow this to be implemented as needed by whatever descendent.
It does add a little virtual call overhead but that's all and now you have buffer support everywhere.
The funny part is that TBufferedFileStrean was lifted from FireDAC which deals with network IO, not files, but still the opportunity to broaden the buffering scope was missed by opting for the quick win.
ReplyDeleteTStream and friends continues to be an excellent example of how not to do it.
ReplyDeleteAsbjørn Heid
ReplyDeletePlease elaborate, you seem to have a number of issues.
Johan Bontes That's what my girlfriend said...
ReplyDeleteJokes aside:
If I pass you a TStream, can you read from it? Write to it? Seek backwards? Resize it? How would you know?
Off the top of your head, what does
function Write(const Buffer: TBytes; Offset, Count: Longint): Longint; overload; virtual;
the Offset parameter do? And Longint?!? Dynamic arrays can be >2GB on x64.
In
procedure ReadBuffer(var Buffer: TBytes; Count: Longint); overload;
you'd expect Buffer to be resized to fit Count right? After all, what other possible reason is it for passing Buffer as var? Protip: it doesn't.
Why oh why are all the component streaming methods part of TStream?
I'm sure I can think of more if it wasn't friday :)
Asbjørn Heid You really do have a number of issues! LOL! More seriously: I do see some of the points raised, but when I first used them in '99 I was just looking to wrap my head around them to get my files and streams written and read out from... didn't question their design then, didn't question them later because I just got used to them... Thanks to all the pros Stefan Glienke who stay alert ;)
ReplyDeleteAsbjørn Heid you're so right. Given the good examples in the JCL streams, the already existing ZLib (de)compression streams and the great examples in STREAMS16.ZIP by Duncan Murdoch, I can only say #facepalm . #again .
ReplyDeleteI think I'll write a blog post in due time.
I don't think that ReadBuffer() should resize the buffer. You may want to read more data into the buffer or have other reasons to make the buffer larger (perhaps you want to store data after the part read).
ReplyDeleteIn other words: I don't want ReadBuffer to tell me how big the buffer should be. It should simply return the size read and let me, the consumer of the code, decide what to do next.
ReplyDeleteRudy Velthuis I agree, however there is no other reason for the parameter to be "var". Of course, changing "var" to "const" would be preferable, but if they want to keep it "var" for some strange reason then resizing the buffer is the only sane thing to do.
ReplyDelete