Friday, September 03, 2010

Ingenious!

Sometimes you find a code fragment that’s so great you only wish it was your idea …
const
  fm: array[Boolean] of Word = (fmCreate, fmOpenWrite);
fs := TFileStream.Create(fileName, fm[FileExists(fileName)]);

Obvious – in retrospective.

14 comments:

  1. Primoz this is really ingenius, but maybe this line

    fs := TFileStream.Create(fileName, fm[FileExists(fn)]);

    should be

    fs := TFileStream.Create(fileName, fm[FileExists(fileName)]);

    ReplyDelete
  2. 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.

    ReplyDelete
  3. @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.)

    ReplyDelete
  4. Victor22:05

    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);

    ReplyDelete
  5. @Victor: I think if the file does not exists then fmOpenWrite will fail.

    ReplyDelete
  6. Olaf10:44

    If one includes "Math.pas" one can use "IfThen" to make it easier readable:
    fs := TFileStream.Create(fileName, IfThen (FileExists (fileName), fmOpenWrite, fmCreate));

    ReplyDelete
  7. If I run into such code I rewrite it on the spot. It is unreadable and thus likely to create bugs.

    Well written code should look like an English novel, not like a bunch of tricks to save a line.

    ReplyDelete
  8. @Victor: It won't work. Your code will always truncate the file to 0 size even if the file exists.

    ReplyDelete
  9. ...or


    function 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 ;-)

    ReplyDelete
  10. 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...

    ReplyDelete
  11. @LDS: With that I *completely* agree!

    ReplyDelete
  12. Anonymous10:14

    I agree with Jan Derk, this code is not easy to read and should be avoided.

    ReplyDelete
  13. 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.

    W

    ReplyDelete
  14. 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/

    Credit goes where credit's due ;)

    ReplyDelete