I’m writing this article because I want to share programming habits with you. What habit did prove to be a good one for you? Share yours then, please.

I always add “RaiseLastOsError” to every Windows API call.

if not ImpersonateLoggedOnUser(token) then
  RaiseLastOsError;

In this way you don’t miss any failed call and wonder what is happening (I can recall a lot of forum threads that ask about this).
RaiseLastOsError generates an EOSError exception that contains the error number and text in its message. It also stores the GetLastError value in its property ErrorCode.

One more good thing is that a caller of the function, where this code is implemented, cannot ignore an error value. The API function ImpersonateLoggedOnUser is very problematic because if it fails all subsequent calls are made on the security token of the process instead of the thread. So a client could access more things than usual. So if we had a function that implements the example

function MyImpersonateUserToken(UserName : String; out UserData : PUserData) : Boolean;
begin
  //get user token here
  //get user information from token here and store it into UserData

  result := ImpersonateLoggedOnUser(token);

As you can see a caller can just do the following

MyImpersonateUserToken(UserName, UserData);
If UserData.Groups.IsMemberOf('Administrators') then

Be aware that the example is only fictional. However I have seen many production codes that call WinAPI without error checking. The WinAPI usually (and the code above definitely) does not touch any of the output parameters if they fail. So you probably get an Access Violation (best case) or it works randomly (worst case). See that UserData is not initialized here. But what about this one:

procedure MyImpersonateUserToken(UserName : String; out UserData : PUserData);
begin
  //get user token here
  if not GetUserToken then
    raiseLastOsError;
  //get user information from token here and store it into UserData
  if not GetTokenInformation then
    raiseLastOsError; 

  if not ImpersonateLoggedOnUser(token) then
    raiseLastOsError;

You see that the function is really a procedure now that throws exceptions in case of failure. In this way the code following the function call MyImpersonateUserToken isn’t executed at all. Instead, we need to wrap an exception trap around the function call to let the code continue.

try
  MyImpersonateUserToken(UserName, UserData);
except
end;

if UserData.Groups.IsMemberOf('Administrators') then

Okay, everbody should see that this is a really ******* code and should not be left in this way.

If you make an habit from this you won’t miss the old way. Instead it takes less time to implement and I don’t mention the time searching for memory leaks.

Furthermore, I think it looks much better and besides of that it is really easy to read and understand.
However, the most important reason (for me) is that any user who calls this function incorrectly (e.g. user not found) gets a nasty exception thrown into the face.

These are the reasons why JWSCL throws exceptions in 99,99% of its methods instead of returning error values.

I always use try/finally for memory allocations

Programmers have a hard to find memory leaks. There are libraries (like JWSCL) that support smart pointers but Delphi comes also with “smart pointers” (besides interfaces). Although it needs some more work. If you have translated code from C to Delphi you have seen it maybe. I’m talking about “goto” statements in C code.

{
  ...
  if (!GetTokenInformation(...))
    goto exit_1;

goto exit_0;
:exit_1
  CloseHandle(hToken);

:exit_0
return (result);
}

In Delphi we don’t use goto because we were told that it is  bad habit. So we directly put the exit after each failed function called.

begin
  ...
  if not GetToken(hToken) then
  begin
    result := true;
    exit;
  end;
  if (not GetTokenInformation(...)) then
  begin
    CloseHandle(hToken);
    result := FALSE;
    exit;
  end;

  CloseHandle(hToken);
  result := TRUE;
end;

In more complex code you can easily miss a failure code path or miss to close a handle or free an object. So why don’t do it this way?

  ...
  if not GetToken(hToken) then
    raiseLastError;

  try
    if (not GetTokenInformation(...)) then
      raiseLastError;

    if GetMeOutOfHere() then
      exit;
  finally
    CloseHandle(hToken);
  end;
end;

The try/finally statement ensures that the code within finally is executed. It does not matter that an exception is raised (we know that already). Even if we call “exit” the finally part is also executed.

However, you need to make sure that the variables within finally are valid.

1. CloseHandle throws an exception if the handle is not valid. This only happens if it detects an attached debugger.

2. Free, FreeAndNil, FreeMem, Dispose will throw an exception depending on the content of the given pointer. In worst case nothing happens and you get garbage results anywhere else in your application.

So the correct way is to trap every allocated variable (pointer, class) in a separate try/finally statement.

New(Ptr);
try
  MyClass := TMyClass.Create;
  try
    ...
  finally
    FreeAndNil(MyClass);
  end;
finally
  Dispose(Ptr);
  Ptr := nil;
end;

Make it an habit to implement the finally part immediately. Delphi helps you by adding the finally part automatically.

New(Ptr);
try
  (1)
finally
  Dispose(Ptr);
  Ptr := nil;
end;

Now continue at position (1).

One more tip: If you have a lot of source lines between try/finally you can also put a comment behind finally to show the initial call.

New(Ptr);
try
  ... many lines ...
finally //New(Ptr);
  Dispose(Ptr);
  Ptr := nil;
end;

In this way you don’t mix up the variable names allocated and freed. And we also don’t mix up the allocation types (e.g. GetMem <> Dispose)


Your programming habits

If you want to share your habits you can do this by leaving a comment. Or better you can write your own article and link it here so a conection is made.

Looking forward to hearing from you!


Christian