3

What is the proper approach to functions that have side-effects in Delphi/Pascal?

For example, I could have a boolean function DeleteFile that returns True if the file was deleted and False otherwise. How do I collect the return value?

1) The very explicit form would be:

if DeleteFile(MyFilePath) then
begin
  IsMyFileDeleted := True;
end;
else
begin
  IsMyFileDeleted := False;
end;

2) Less bulky would be:

IsMyFileDeleted := False;
if DeleteFile(MyFilePath) then
begin
  IsMyFileDeleted := True;
end;

3) Or even shorter, but discouraged by JEDI - perhaps it is acceptable just to collect return values:

IsMyFileDeleted := False;
if DeleteFile(MyFilePath) then IsMyFileDeleted := True;

My problem with 2 and 3 is that I explicitly state something (X := False) that I may not know for sure.

4) I would rather not set the variable beforehand and have its value completely determined by the function:

IsMyFileDeleted := DeleteFile(MyFilePath);

Here the issue is that it is a bit misleading, because assigning a value to a boolean has a pretty serious side-effect. I'm not sure if this usage is desirable.

Some argue that functions should never have side-effects, but I think some of Pascals internal functions violate that.

5) The alternative would be making it procedures with a result variable to be filled by the procedure:

DeleteFile(MyFilePath, IsMyFileDeleted);

That might go against the trend to keep parameter use to a minimum (Clean Code).

4 and 5 would be the only ways to collect a return value other than boolean, I suppose? Does that mean anything for boolean functions?

Yes, there are many ways to do the same thing and maybe I'm just worried about nothing really, but I'm trying to develop a good style and I wonder if there are any best practices. For a beginner, many principles seem to be in conflict all the time.

gnat
  • 21,442
  • 29
  • 112
  • 288
thoiz_vd
  • 131
  • 2
  • 6
  • 2
    The more serious problem I see is one with `DeleteFile`, namely that its name tells you nothing about the return value. Ignoring that and assuming a reasonable name, I don't see how 4 is unreadable. The assignment doesn't have any side effect (apart from changing the variable, of course). –  Sep 19 '11 at 17:00
  • @delnan: suggestions? I thought it would be safe to assume that such a function reports success or failure. My point was that the side-effect of determining if a file is deleted, is actually deleting it. – thoiz_vd Sep 19 '11 at 17:04

6 Answers6

11

Option #4 is fine, really. Delphi isn't a functional programming language and doesn't really try to be one, so talking about "side effects" in this context is kind of silly.

Deleting the file isn't "a side effect" of calling DeleteFile, it's simply the effect of calling it. You don't call it to get a boolean, you call it to delete a file. If anything, the "side effect" is returning a result at all.

Mason Wheeler
  • 82,151
  • 24
  • 234
  • 309
4

I think you are making things way too hard for yourself.

There are many functions in the VCL and the Windows API that are only functions because they return success or failure and only functions can return anything. To all intents and purposes though these functions are procedures in what they do.

If they didn't return their success or failure as a function result, you would have to jump through hoops to check whether they were successful, or would have to rely on them throwing exceptions if they just weren't able to do what they are meant to do but didn't encounter any real problems:

try
  DeleteFile(MyFilePath);
  IsMyFileDeleted := True;
except
  IsMyFileDeleted := False;
end;

You really don't want to litter your code with constructs like that. Plus, especially on win32, exceptions are expensive. And, using exceptions for flow control is not good practice anyway.

So, use #4

IsMyFileDeleted := DeleteFile(MyFilePath);

It does exactly what you need and it is pretty obvious that you are just capturing the success or failure of the DeleteFile.

And if you have a "function that really is a procedure reporting success or failure" through error codes, you could also do something like:

IsThisSuccessful := (ProcedureAsFunction = S_OK);

Marjan Venema
  • 8,151
  • 3
  • 32
  • 35
2

In delphi, one of the primary differences between procedures and functions is that a function always has a return value, and for each input, you will get one and only one return value out, no matter how many times you run the function on the same input.

So, for your code, #4 is fine, so long as it returns the same value on every run of the same input. So if you run the delete function on a file and return true, then rerun the function on the same file, it should return true again. Even though the file is already gone! The current runtime state should have no impact on your return values in delphi functions. Unless this is implied by the function use states and/or documentation. For a good example, see John's comment below.

If the current state is important, then you should be using a procedure which mutates an input boolean variable, or better, a record that can return more information, such as whether the file exists, and if we have permission to delete.

Or, you should add inputs to your function, to allow for more variation. A good one would be what to do if the file doesn't exist. Finally, if you just want to include current state implicitly, make sure you document that fact.

Spencer Rathbun
  • 3,576
  • 1
  • 21
  • 28
  • 2
    Odd, then, that the [`SysUtils.DeleteFile`](http://docwiki.embarcadero.com/VCL/en/SysUtils.DeleteFile) function in Delphi returns a boolean in exactly the way that the OP suggests. And clearly the [`System.Random`](http://docwiki.embarcadero.com/VCL/en/System.Random) function can't be much use if it returns the same value every time. – John Bartholomew Sep 19 '11 at 20:10
  • Of course, it's not a hard and fast rule. But in general, this is one of the primary differences between functions and procedures. Others being that functions do not mutate their inputs, and procedures do not have a return value. In fact, your `SysUtils.DeleteFile` example is a better function, as it has documentation listing exactly what it does, and noting that *file existence* is an implicit input. – Spencer Rathbun Sep 19 '11 at 20:25
  • Specifically, `SysUtils.DeleteFile` returns *function* sucess or failure, *not* whether the file got deleted. You can imply that from True, but not from False, which could be misleading. – Spencer Rathbun Sep 19 '11 at 20:34
  • Functions in pascal / Delphi can return further values by means of var parameters, in addition to the return value. The names procedure and function only determine whether there is a Result, or not, i.e. whether the return value can be used in an expression or not. There are not limitations on side-effects at all. – Reversed Engineer Feb 19 '16 at 06:01
1

Use #4,

IsMyFileDeleted := DeleteFile(MyFilePath);

Or possibly, since this is Delphi,

IsMyFileDeleted := false;
try 
   IsMyFileDeleted := DeleteFile(MyFilePath);
except
   ...
end;

If you want to be properly paranoid about it, if you are concerned an exception might be thrown. I dont think you are achieving anything in the other options other than making simple code unnecessarily complicated.

GrandmasterB
  • 37,990
  • 7
  • 78
  • 131
0

To be clearer about what's going on, you should probably do something like this:

IsMyFileDeleted := False;
if (CanDeleteFile(MyFilePath)) then
begin
    DeleteFile(MyFilePath);
    IsMyFileDeleted := True;
end

The first line is true at the point it has been run (unless the file doesn't exist).

Using CanDeleteFile(...) allows you to separate the true/false features of the code's intent from the exceptional behavior of being unable to delete something that should have been deleted. So, if you can delete the file, then DeleteFile should raise an exception if it can't.

Edited

If I were writing in C#, the first situation would have looked like this. Using exception handling would likely be better in Delphi/Pascal, if available. In this model, DeleteFile() returns nothing at all. Either it works, or it supplies information about why it didn't work.

try
{
    DeleteFile(MyFilePath);
}
catch (IOException ex)
{
    // Do whatever you would have done if DeleteFile() returned false.
}
John Fisher
  • 1,795
  • 10
  • 12
  • Is it realistic to think that it is always predictable if something can be done? After all this is just one specific example. – thoiz_vd Sep 19 '11 at 17:06
  • @thoiz_vd: The idea is to make exceptional situations exceptional, and normal situations visible in the code. If DeleteFile() should always work, then why return a true/false value at all? If it is not completely abnormal for DeleteFile() to fail, then this pattern should work well. – John Fisher Sep 19 '11 at 18:20
  • This is usually a bad idea: now you have two code paths (if `CanDeleteFile` returns false, and if `DeleteFile` throws an exception) for one logical situation (the file could not be deleted). And you *must* handle the possible exception from `DeleteFile`, because your application is running concurrently with other applications that can modify the file-system (for example, another application could rename the file between your call to `CanDeleteFile` and `DeleteFile`). – John Bartholomew Sep 19 '11 at 18:21
  • @John Fisher: Whether failure to delete a file is exceptional depends on the context. I believe a better suggestion is to have two functions: `TryDeleteFile` that returns a success/failure value, and `DeleteFile` which returns nothing but throws an exception on failure. Then you can use whichever is appropriate in the context, which is not something that is known by those functions. – John Bartholomew Sep 19 '11 at 18:24
  • @JohnBartholomew: Look at my edits. Also, consider the common patterns in the .NET framework around this exact type of operation. (File.Exists(), File.Delete(), and others). – John Fisher Sep 19 '11 at 18:27
  • @JohnFisher: I have no problem with `DeleteFile` throwing an exception on failure. My argument was with your attempt to somehow enforce a distinction between an exceptional and non-exceptional failure, without some application-specific rules (clearly such rules are easy to think of, for example "failure because the file doesn't exist is non-exceptional and can be ignored; failure because you have incorrect file permissions is exceptional and should be reported to the user", but equally clearly, those rules do not belong at the level of a `DeleteFile` library function itself) – John Bartholomew Sep 19 '11 at 18:34
  • @JohnBartholomew: To make that decision you first have to assume that you know what DeleteFile() is doing... It seems like your argument becomes self-defeating. Clearly, the one writing or using the function has the required information to make the decision. – John Fisher Sep 19 '11 at 18:59
  • let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/1378/discussion-between-john-bartholomew-and-john-fisher) – John Bartholomew Sep 19 '11 at 19:44
  • @JohnBartholomew: The `DeleteFile` case is interesting because there are some usage scenarios where it's expected that the file should have existed before the delete, and if it didn't exist that would mean something was wrong, and there are other usage scenarios where what matters is that after the delete no file of the indicated name should exist; if that happened to be true before the call, it would naturally be true after, and thus the call should "successfully" delete zero files [btw, a useful return value might be the number of files deleted (possibly zero) if successful; else throw]. – supercat Nov 03 '14 at 21:18
-2

There is often an error message that you need to pass up the chain.

Exceptions are violent, time-consuming structures. So rather than have exception handling all over the place, I have started to use a String return instead of Boolean. If you still need a flag then just check the string for empty/blank.

var Error : string;

begin
  Error := DeleteFile(MyFilePath);
  IsMyFileDeleted := Error = '';
Rohit Gupta
  • 203
  • 2
  • 3
  • 12