Jump to content

Recommended Posts

Posted (edited)

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 by Sherlock
Slightly better title

Share this post


Link to post

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

Share this post


Link to post

Does the behavior change if you write

If not aUser.Administrator then? 

Share this post


Link to post

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. 

  • Like 6

Share this post


Link to post

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.

Share this post


Link to post

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?

Share this post


Link to post
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? 

  • Like 2

Share this post


Link to post

Solved 🙂

 

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

 

Regards,

Ole

Share this post


Link to post

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

  • Like 3

Share this post


Link to post
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 🙂

Share this post


Link to post
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?

  • Like 2

Share this post


Link to post

It's simple. Why write 

 

if SomeBool = True then 

 

when you can write 

 

If SomeBool then

 

It simply adds nothing. 

  • Like 2

Share this post


Link to post

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.

 

  • Like 3

Share this post


Link to post

Call it: .IsAdministrator and all is clear

  • Like 2

Share this post


Link to post

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.

  • Like 1

Share this post


Link to post
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!:classic_laugh: 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?

Share this post


Link to post

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. 

 

  • Like 1

Share this post


Link to post

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;

 

Share this post


Link to post

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;

 

Share this post


Link to post
Posted (edited)

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 by Bill Meyer
  • Like 1

Share this post


Link to post

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.

Share this post


Link to post
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;

 

 

 

 

 

 

 

 

 

 

  • Like 2

Share this post


Link to post
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.

Share this post


Link to post
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

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×