Actually not. I believe this a good way to write bad code. It should be easy to read, maintain, fix and document. An unexperienced developer can easily got trapped in this cool looking snippet and start using it whenever he needs readonly access for example.
Frankly - I like the idea but just as an idea. I will never use it on my own.
@thereoadtodelphi: Of course. I was pretty-formatting the code and missed this part.
@Gad D Lord: You have to know when to use it and when not. That's a typical code fragment that I would use in debugging code. Logging, for example. (And that's exactly where the fragment came from.)
I think this is actually a case of not being sure what argument to pass when you don't care whether the file already exists or not. Testing with FileExists is not necessary at all! The flags may simply be combined like this:
fs := TFileStream.Create(fileName, fmCreate or fmOpenWrite);
If one includes "Math.pas" one can use "IfThen" to make it easier readable: fs := TFileStream.Create(fileName, IfThen (FileExists (fileName), fmOpenWrite, fmCreate));
function ForceFileStream(const FN: String): TFileStream; begin if FileExists(FN) then Result := TFileStream.Create(FN,fmOpenWrite) else Result := TFileStream.Create(FN,fmCreate); end;
That's just because Delphi still doesn't support the OPEN_ALWAYS flag of CreateFile()? One of the issues of the VCL is it never keeps current with the Windows API...
there are several reasons to avoid complex one liners: 1. harder to understand what is happening. 2. harder to debug. no local variables to inspect. 3. it is not obvious how this will fail, making it harder to fix.
If such a pattern was needed in many places, I would think a CreateOrOpenStream local function would be a much better one-liner. I love clever code, just not in my production applications.
I love posts like this. Goes to show there is always something to learn. @Victor: "fs := TFileStream.Create(fileName, fmCreate or fmOpenWrite);" .. I tried that a few times. It strikes me that my bitmask may have failed because i added "ShareDenyNone" to the mix, which broke the word boundary.
I added this to my article @ http://delphimax.wordpress.com/2011/02/02/smarting-up-your-classes-lookup-tables-and-how-to-use-them/
Primoz this is really ingenius, but maybe this line
ReplyDeletefs := TFileStream.Create(fileName, fm[FileExists(fn)]);
should be
fs := TFileStream.Create(fileName, fm[FileExists(fileName)]);
Actually not. I believe this a good way to write bad code.
ReplyDeleteIt should be easy to read, maintain, fix and document.
An unexperienced developer can easily got trapped in this cool looking snippet and start using it whenever he needs readonly access for example.
Frankly - I like the idea but just as an idea. I will never use it on my own.
@thereoadtodelphi: Of course. I was pretty-formatting the code and missed this part.
ReplyDelete@Gad D Lord: You have to know when to use it and when not. That's a typical code fragment that I would use in debugging code. Logging, for example. (And that's exactly where the fragment came from.)
I think this is actually a case of not being sure what argument to pass when you don't care whether the file already exists or not. Testing with FileExists is not necessary at all! The flags may simply be combined like this:
ReplyDeletefs := TFileStream.Create(fileName, fmCreate or fmOpenWrite);
@Victor: I think if the file does not exists then fmOpenWrite will fail.
ReplyDeleteIf one includes "Math.pas" one can use "IfThen" to make it easier readable:
ReplyDeletefs := TFileStream.Create(fileName, IfThen (FileExists (fileName), fmOpenWrite, fmCreate));
this would be a preffered manner for those reading the code for clarity.
DeleteIf I run into such code I rewrite it on the spot. It is unreadable and thus likely to create bugs.
ReplyDeleteWell written code should look like an English novel, not like a bunch of tricks to save a line.
@Victor: It won't work. Your code will always truncate the file to 0 size even if the file exists.
ReplyDelete...or
ReplyDeletefunction ForceFileStream(const FN: String): TFileStream;
begin
if FileExists(FN) then
Result := TFileStream.Create(FN,fmOpenWrite)
else
Result := TFileStream.Create(FN,fmCreate);
end;
fs := ForceFileStream(FileName);
Not tested, of course ;-)
That's just because Delphi still doesn't support the OPEN_ALWAYS flag of CreateFile()?
ReplyDeleteOne of the issues of the VCL is it never keeps current with the Windows API...
@LDS: With that I *completely* agree!
ReplyDeleteI agree with Jan Derk, this code is not easy to read and should be avoided.
ReplyDeletethere are several reasons to avoid complex one liners:
ReplyDelete1. harder to understand what is happening.
2. harder to debug. no local variables to inspect.
3. it is not obvious how this will fail, making it harder to fix.
If such a pattern was needed in many places, I would think a CreateOrOpenStream local function would be a much better one-liner. I love clever code, just not in my production applications.
W
I love posts like this. Goes to show there is always something to learn. @Victor: "fs := TFileStream.Create(fileName, fmCreate or fmOpenWrite);" .. I tried that a few times. It strikes me that my bitmask may have failed because i added "ShareDenyNone" to the mix, which broke the word boundary.
ReplyDeleteI added this to my article @ http://delphimax.wordpress.com/2011/02/02/smarting-up-your-classes-lookup-tables-and-how-to-use-them/
Credit goes where credit's due ;)