Boolean evaluation

[Ole Ekerhovd 0]

Delphi 10.3

I have a code snippet that is not working as I expect.

 

 // Check if active user is different from document owner

  if aUser.Username<>aDocument.Owner then
  begin

    // If user is not an administrator
    if aUser.Administrator=false then
    begin
      showmessage('You are not allowed to delete this document');
      exit;
    end;
  end;

 

Problem: the showmessage pops up even if the active user is an administrator and the boolean Administrator is True

 

What anm I missing here?

 

Ole

Edited March 24, 2020 by Sherlock
Slightly better title

[Kryvich 165]

What is Administrator? Is it a field, or a method (function)? What actual type does this field have?

[Lars Fosdal 1792]

Does the behavior change if you write

If not aUser.Administrator then? 

[David Heffernan 2345]

Never compare to False or True. Instead write 

 

if someBool then

 

Or 

 

if not someBool then

 

That's not your problem though. Your problem is quite simple. It's the most obvious explanation. You are mistaken, in fact aUser.Administrator is false. It's that simple. 

[Ole Ekerhovd 0]

Kryvich: aUser.Administrator is a boolean field in a record structure.

 

Lars: No, even if I do

aUser.Administrator:=true;

if aUser.Administrator=false then
begin .... end;

 

the begin end always execute.

[Guest]

CPU Window? Trace into aUser.Administrator read. Does not sound like a boolean record member. Oh, and... do you have "with" somewhere above that code?

[David Heffernan 2345]

1 hour ago, Ole Ekerhovd said:

aUser.Administrator=false

Why is it so hard to reach the conclusion that aUser.Administrator is false? 

 

Instead of being helpless here, you can get help from us by providing a minimal reproduction. Easier still would be simply to debug your program. Have you done that? 

[Ole Ekerhovd 0]

Solved 🙂

 

It was a logical error in the code, and really hard to catch too.

 

Regards,

Ole

[Lars Fosdal 1792]

I have to admit that I cringe whenever I see a boolean compared to true or false.

[Ole Ekerhovd 0]

41 minutes ago, Lars Fosdal said:

I have to admit that I cringe whenever I see a boolean compared to true or false. 

Why?

 

Please elaborate so I can learn from it 🙂

[Dave Nottage 557]

49 minutes ago, Ole Ekerhovd said:

Please elaborate so I can learn from it

That was exactly my thought when you made this comment:

5 hours ago, Ole Ekerhovd said:

It was a logical error in the code, and really hard to catch too.

What was the error, and why was it hard to catch?

[David Heffernan 2345]

It's simple. Why write 

 

if SomeBool = True then 

 

when you can write 

 

If SomeBool then

 

It simply adds nothing. 

[Lars Fosdal 1792]

My argument comes from the goal of reducing code clutter and sanitizing conditions to be easy to read.

 

Original code

if aUser.Username<>aDocument.Owner then
  begin
    // If user is not an administrator
    if aUser.Administrator=false then
    begin
      showmessage('You are not allowed to delete this document');
      exit;
    end;
  end;

 

A boolean is by definition either true or false.   An if condition takes a boolean argument, and if the argument is true, the following statement is executed.

I don't need to check if BoolVar = true or BoolVar = false - only checking BoolVar or not BoolVar is enough.  

 

To have readable code, avoiding negations is a good thing, hence I would probably write the above something like this

if (aUser.Username = aDocument.Owner) or aUser.Administrator 
then begin
  // do the deletion
end
 else ShowMessage('You are not allowed to delete this document');

Naturally, there is no single right way of doing conditions - but explicitly comparing a boolean variable to a boolean value is unnecessary.

 

[Rollo62 536]

Call it: .IsAdministrator and all is clear

[Bill Meyer 337]

My roots were in hardware logic. Excess terms in hardware translates to using unnecessary parts, each with a measurable cost. Logic reduction is as important in software as in hardware. We don't see a hard cost for the unnecesary terms, but the cost is there, nonetheless. Look at the simplified form Lars presents. Not a big deal in small cases, but in complex chains of logic, failure to minimize becomes a maintenance issue; it impedes understanding.

[Ole Ekerhovd 0]

1 hour ago, Dave Nottage said:

That was exactly my thought when you made this comment:

What was the error, and why was it hard to catch?

  A very, very idiotic mistake. The same check was also done from another unit, I did not remember it. That routine had the exact same text in Showmessage and that routine had a bug and checked against True, not False! So it was not the error message in the routine I posted here that was shown. A lot of hours lost, but I guess this may happen when a program grow to hundreds of units?

[David Heffernan 2345]

It would have been trivial to solve if only you'd used the debugger. Make it your next mission to learn how to use the debugger. 

 

[Lars Fosdal 1792]

If the same logic is used in multiple places, that shouts for generalization.

function UserCanDeleteDocument(const aUser: TUser; const aDoc: TDoc): boolean;
begin                                     // can delete if
  Result := (aUser.Username = aDoc.Owner) // aUser owns ADoc
          or aUser.Administrator          // or aUser has admin rights
end;

begin
  if UserCanDeleteDocument(aUser, aDoc)
  then begin
    // do the deletion
  end
   else ShowMessage('You are not allowed to delete this document');
end;

 

[Lars Fosdal 1792]

A "modern" way of handing conditions is to do early exit on condition fail.  In the above case it is pointless, but for more complex sequences it can make sense.

Here is an example from a piece of JsonRPC handling code I wrote.
In this case I don't do exits - since there is stuff that happens after the validation.

Imagine this code if I had used nested if/then...

        // Handle login progress errors

        if Result and Session.Progress.Device.Required
        then begin
          if Session.Progress.Device.FailAsError
           then Response.SetError(Session.Progress.Device);
          Response.SetHint(Session.Progress.Device.Message);
          Response.SelectNextStep(S01_Login);
          Result := False;
        end;

        if Result and Session.Progress.User.Required
        then begin
          if Session.Progress.User.FailAsError
           then Response.SetError(Session.Progress.User);
          Response.SetHint(Session.Progress.User.Message);
          Response.SelectNextStep(S02_SelectUser);
          Result := False;
        end;

        if Result and Session.Progress.Mission.Required
        then begin
          if Session.Progress.Mission.FailAsError
           then Response.SetError(Session.Progress.Mission);
          Response.SetHint(Session.Progress.Mission.Message);
          Response.SelectNextStep(S03_SelectMission);
          Result := False;
        end;

        if Result and Session.Progress.Truck.Required
        then begin
          if Session.Progress.Truck.FailAsError
           then Response.SetError(Session.Progress.Truck);
          Response.SetHint(Session.Progress.Truck.Message);
          Response.SelectNextStep(TPGStep.S04_SelectVehicle);
          Result := False;
        end;

        if Result and Session.Progress.Area.Required
        then begin
          if Session.Progress.Area.FailAsError
           then Response.SetError(Session.Progress.Area);
          Response.SetHint(Session.Progress.Area.Message);
          Response.SelectNextStep(TPGStep.S05_SelectArea);
          Result := False;
        end;

 

[Bill Meyer 337]

Another suggestion I offer is that when you must evaluate more than a few terms, it can be helpful to make use of local variables. This is especially true where each of your terms is itself lengthy, such as SomeObj.SomeMember.AProperty. Similarly, if several terms evaluate properties in a complex object, then rather than writing:

if SomeObj.SomeMember.TermOne and SomeObj.SomeMember.TermTwo and SomeObj.SomeMember.TermThree then

 

You might instead do:

var

  obj: TMySubClass;

begin

  obj := SomeObj.SomeMember;

  if obj.TermOne and obj.TermTwo and obj.TermThree then

 

I also find that with many terms and long reference names, the odds of making errors in precedence increase rapidly. And the reason for that is that the mess in front of your eyes is approaching incomprehensibility. 😉

Edited March 21, 2020 by Bill Meyer

[Lars Fosdal 1792]

Absolutely. The Law of Demeter is something I usually follow quite strictly. 

However, in the above example - Session.Progress.Truck.Required ( & alike),  Session has already been validated and Progress is an object where all the fields are always populated. 
I actually did make this structure instead of having a buttload of local variables. 

I have to figure out if a value has been given, and if it is given, it is valid, and if not - if it is a required value, and so forth, and some login types may need only some of the parameters.

[A.M. Hoornweg 144]

On 3/21/2020 at 6:52 PM, David Heffernan said:

It's simple. Why write 

 

if SomeBool = True then 

 

when you can write 

 

If SomeBool then

 

It simply adds nothing. 

Not only that, but the code "if Somebool=True" has a little known pitfall.   

 

Every Delphi program interfaces with libraries written in C or C++ (such as the Windows API, all sorts of drivers etc) and in the C language, a bool is basically an INT.  I have had a few cases where a function was supposed to return a boolean and in reality it returned some integer where 0 signalled FALSE. 

 

The code (if Somebool Then...) treats 0 as false and all other values as TRUE.  That always works as intended.  The code "If Somebool=True THEN ..." only treats 1 as TRUE and everything else as FALSE. 

CASE statements are affected, too.

 

So the following code fails:

var SomeBool:boolean;
begin
   SomeBool:=Boolean(2); //Simulate a DLL returning a "wonky" boolean   
   if (SomeBool=True) then ShowMessage('True');
   CASE SomeBool of
     True: ShowMessage('True');
     False:ShowMessage('False');
   END;
   If Somebool then ShowMessage('Still it is true...');
end;

 

 

 

 

 

 

 

 

 

 

[David Heffernan 2345]

This point about interop has merit, however, Boolean is the wrong type for interop. You need LongBool. 

[A.M. Hoornweg 144]

3 hours ago, David Heffernan said:

This point about interop has merit, however, Boolean is the wrong type for interop. You need LongBool. 

In the Windows API, sure.

[Fr0sT.Brutal 900]

22 hours ago, A.M. Hoornweg said:

The code "If Somebool=True THEN ..." only treats 1 as TRUE and everything else as FALSE.  

Meanwhile BoolToStr returns '-1' for True %-)

On 3/21/2020 at 10:44 PM, Lars Fosdal said:

Here is an example from a piece of JsonRPC handling code I wrote.
In this case I don't do exits - since there is stuff that happens after the validation.

"finally" sections are good for making early exits that must not exit the subroutine