Delphi: do not do “if (not DirectoryExists(path)) then ForceDirectories(path))”
Posted by jpluimers on 2013/07/26
During code reviews, I often see people do things like this:
if (not DirectoryExists(Path)) then ForceDirectories(Path))
or this:
if (not TDirectory.Exists(Path)) then TDirectory.CreateDirectory((Path))
Half a year ago, I wrote about .NET/C#: do not do “if (!Directory.Exists(path)) Directory.CreateDirectory(path))”.
The same holds for since Delphi XE introduced the ForceDirectories method in the SysUtils and FileCtrl units and the TDirectory.CreateDirectory method in the IOUtils unit.
You don’t need the if statements here in Delphi either: The methods ForceDirectories and TDirectory.CreateDirectory (that internally calls ForceDirectories) will do nothing if the directory already exists
So your code only needs to be like this:
ForceDirectories(Path));
or this:
TDirectory.CreateDirectory((Path));
This is how a simplified ForceDirectories method implementation looks like:
function ForceDirectories(Dir: string): Boolean;
begin
Result := True;
if Dir = '' then
raise EInOutError.CreateRes(@SCannotCreateDir);
Dir := ExcludeTrailingPathDelimiter(Dir);
if (Dir = '') or DirectoryExists(Dir) then Exit;
Result := ForceDirectories(ExtractFilePath(Dir)) and CreateDir(Dir);
end;
–jeroen
The Wiert Corner - irregular stream of stuff 
Dennis said
I really don’t see *any* problem in writing such code. It works and makes the code more readable and isn’t a big performance hog (you’re not calling this in a loop, aren’t you?). The really interessting part is the UNC problem which Marius showed to you – that’s really important while using DirectoryExists() and not if you add this useless if-statement…
jpluimers said
Less code means less clutter, and less chances to introduce mistakes. And I think DRY applies here as well: it is a repetition you don’t need.
Note that the `DirectoryExists` bug is fixed in Delphi XE2 and up.
David Heffernan said
It’s a bit weak that the library code calls DirectoryExists at all. Surely it should try to create the directory every time. And then gracefully handle the error that occurs if the directory already exists. The system will also be checking whether or not the directory exists. Your post then becomes, don’t check three times, check only twice. ;-)
jpluimers said
I’m not sure if the smiley is for your whole comment, so I regard it as a joke.
Remember the case for slow Delphi IDE startup? They used to load all module translations in a loop containing try-except blocks for every load. Replacing that with a check if the translated module existed before loading it caused a large performance gain.
Error handling (both exceptions and in the Windows API) can be really slow. Not as slow as a failed module load, but still noticeable. So I understand the DirectoryExist call there.
salvadorbs said
Nice post. Thank you. :)
Jørn E. Angeltveit said
I find such cases all over the place too :-)
You should write a follow-up on how to get rid of those too.
I usually use a macro like this:
1) Perform a search for “if (not DirectoryExists(”
2) Start recording
3) CTRL + Y (to delete line)
4) DELETE + DELETE (to inindent the line below)
5) CTRL + F (to jump to next instance)
6) Stop recording
The reason behind the search at the _end_ of the macro script is that I can verify that I really want to perform the editing of these two lines…
If “yes”: CTRL+SHIFT+P (to execute the macro)
If “no”: CTRL+L (to move to next instance
I’m not sure if this is the best way, and it’s not multi-unit. Each unit must be opened individually.
jpluimers said
This is very similar to how ibdonthe macro recordings for solving such issues.
Marius said
Watch out with UNC paths as te DirectoryExists() function is broken in the XE release..
http://qc.embarcadero.com/wc/qcmain.aspx?d=97555
SysUtils.DirectoryExists returns True for non-existing UNC paths
jpluimers said
Thanks; good to know that.
Marco Cantu said
Guess you need to fix the final version of the code… as it reads like the “broken” one…
Marco Cantu said
Sorry, see it is fixed now, didn’t refresh
EMB said
Ok. You say to don’t use:
if (not DirectoryExists(Path)) then
ForceDirectories(Path))
and instead use:
if (not DirectoryExists(Path)) then
ForceDirectories(Path))
Wait… what?
jpluimers said
Sorry for the mishap. This seems to be one of the few cases where I the WordPress “bogus autosave message” I posted about here http://en.forums.wordpress.com/topic/bogus-view-autosave-message?replies=3#post-1304197 wasn’t so bogus.
So the autosave had the code fragements different, but since the autosave message is bogus 99% of the times, I ignored it. That was mistake 1. Normally not fatal. But mistake 2 was fatal: not checking if the scheduled blog post was in fact completely correct.
Thanks for all the notifications: I now fixed the code. Again (:
–jeroen
Rob said
I don’t see a difference between the “good” and “bad” code….
Jürgen Krämer said
I guess you wanted to write the 3rd and 4th example without the if statements.
Matthias said
And what’s the difference?
if (not DirectoryExists(Path)) then
ForceDirectories(Path))
looks like
if (not DirectoryExists(Path)) then
ForceDirectories(Path))
Simply used copy & paste?