8
\$\begingroup\$

Below snippet is part of a game application's logic and the Update function is called every frame (about 60 times a second).

void Update()
{
    handleInputEvents();
    if (bShow == true)
    {         
        if (bOnce)
        {
            button.show(true);
            bOnce = false;
        }
    }
    else
    {
        if (bOnce == false)
        {
            button.show(false);
            bOnce = true;
        }
    }
}  

Is there a better way to do what the above snippet is doing?

What the snippet is trying to achieve?
This snippet uses a button.show() function to update a virtual button's state. Everytime a physical button is pressed the state of virtual button is set to true(where true means the virtual button shows on screen and false means it doesn't) , as the physical button press is released, the state of the virtual button needs to be updated to false, meaning the virtual button will not show on the screen.

Problem: If bOnce flag check is not used, then the button.show() will be called every frame leading to more CPU cycles wasted.

The snippet shows my attempt to solve the problem.

\$\endgroup\$
16
  • 5
    \$\begingroup\$ When you need help with performance issues than you need to add more code, because the bottleneck may not be in the function you are showing us. Please post at least the entire class this code is a member of. Unlike Stack Overflow on Code Review we want to see more code rather than less code, please read A guide to Code Review for Stack Overflow users \$\endgroup\$
    – pacmaninbw
    Sep 23, 2023 at 14:16
  • 1
    \$\begingroup\$ Where is bShow defined? Can you show us the code where you are setting it? \$\endgroup\$
    – Bergi
    Sep 24, 2023 at 4:35
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply summarise the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Sep 24, 2023 at 14:15
  • 1
    \$\begingroup\$ @BCdotWEB Oh I thought the b stands for "button" :P \$\endgroup\$
    – Bergi
    Sep 24, 2023 at 16:42
  • 1
    \$\begingroup\$ @Bergi the b in bShow stands for it's type which is bool, and it's defined as part of the class member. Also to answer your concern, yes because the cpu cycles are used in calling a function, if the logic is present there then it's not very useful as the call instruction will be utilized and then there will be a return instruction taking up significant CPU cycles instead if I just check before calling the function then I can avoid using of CPU cycles every frame \$\endgroup\$ Sep 25, 2023 at 5:16

8 Answers 8

17
\$\begingroup\$

Can I lessen the use of boolean flag variables in this snippet?

Consider 1st comparing.

// bShow bOnce  Action
//   0     0    button.show(false); bOnce = true;
//   0     1    nothing
//   1     0    nothing
//   1     1    button.show(true); bOnce = false;
void Update() {
    handleInputEvents();
    if (bShow == bOnce) {         
      button.show(bShow); // or button.show(bOnce) whichever you find more clear.
      bOnce = !bOnce;
    }
}  
\$\endgroup\$
6
  • 2
    \$\begingroup\$ Although button.show(bOnce++); is even less than button.show(bOnce); bOnce = !bOnce;, it looks strange to increment a bool, so I do not recommend it. \$\endgroup\$ Sep 23, 2023 at 16:56
  • 5
    \$\begingroup\$ I would recommend button.show(bShow) over button.show(bOnce) though. Sure, they're guaranteed to be the same by the if condition, but still bShow is the intended button state and bOnce is just an optimisation. \$\endgroup\$
    – Bergi
    Sep 24, 2023 at 4:34
  • 3
    \$\begingroup\$ I would change the name of bOnce to something like bVisible (and invert its value). That way you have one variable tracking the "desired" state, and another tracking the "actual" state. Then it's natural to write "if (actual_state != desired_state) { set_state(desired_state); actual_state=desired_state; }". \$\endgroup\$
    – psmears
    Sep 25, 2023 at 13:48
  • 1
    \$\begingroup\$ @psmears OP is quiet on how bShow, bOnce are determined. Rather than that incremental change, I'd go for something larger. \$\endgroup\$ Sep 25, 2023 at 17:03
  • 1
    \$\begingroup\$ @chux: Fair enough, though actually I'd argue that your suggestion there is just one or two more incremental changes on top of what you have here plus the one I suggest... \$\endgroup\$
    – psmears
    Sep 25, 2023 at 21:51
10
\$\begingroup\$

Each call to handleInputEvents() can set bShow to a new boolean value.

We could approach the display update task by making a truth table.

| show | once  ||     XOR |     bothTrue | bothFalse | bT || bF |
|------+-------++---------+--------------+-----------+----------|
|    0 |    0  ||       0 |            0 |         1 |        1 |
|    0 |    1  ||       1 |            0 |         0 |        0 |
|    1 |    0  ||       1 |            0 |         0 |        0 |
|    1 |    1  ||       0 |            1 |         0 |        1 |

Looks like you want

    if (!(bShow ^ bOnce))
    {         
        button.show(bShow & bOnce);
        bOnce = !(bShow & bOnce);
    }

But those last two lines are already conditional, so this suffices:

    {         
        button.show(bShow);
        bOnce = !bOnce;
    }

Now the meaning is far more apparent. We're simply copying, then flipping. And what does the XOR mean? It indicates "nothing changed", so its negation is "something changed" which the if body must deal with.


The bOnce state variable seems a clumsy way to express Author's Intent. Yes, I understand that you want to call button.show() just once per change. But why not express that notion directly?

Let bDisplayed cache the most recent .show() value we sent to the button. Our goal is to keep the button in sync with bShow.

    handleInputEvents();
    if (bDisplayed != bShow)
    {         
        bDisplayed = bShow;
        button.show(bDisplayed);
    }

Now it is transparently obvious to the Gentle Reader that bDisplayed closely tracks bShow, and what is displayed always matches bDisplayed. Keep the reader in mind when writing code.

Programs must be written for people to read, and only incidentally for machines to execute.

-- SICP, by Abelson and Sussman

\$\endgroup\$
2
  • 2
    \$\begingroup\$ I do find bDisplayed == bShow to be quite easier to read than !(bDisplayed ^ bShow)... Really. \$\endgroup\$ Sep 24, 2023 at 15:12
  • 4
    \$\begingroup\$ plusGood for renaming variables into the UI domain. ALL refactorings not doing this are just "rearranging the deck chairs on the Titanic" so to speak; more tidy but not addressing the fundamental problem - obscuring understanding of just what is going on. \$\endgroup\$
    – radarbob
    Sep 24, 2023 at 19:24
10
\$\begingroup\$

I'm not familiar with unity so, I can't review your code from that perspective. But we can certainly make this code more concise. Let me show you how to get started with refactor this code.

1. Get rid of nested branching

Instead of having two levels of branching you can combine the conditions like this:

if (bShow && bOnce)
{         
    button.show(true);
    bOnce = false;
}

2. Transform your else and if blocks to else if

It is basically the same thing what we did in the previous step

else if (!bShow && !bOnce)
{
    button.show(false);
    bOnce = true;
}

3. Take a step back

Now we have only two branches so, it is much easier to overview what we have:

  • If both variables are true then show button and set bOnce to false
  • If both variables are false then hide button and set bOnce to true
  • Otherwise do nothing

4. Combine the conditions

We can combine the two conditions by saying either both true or both false

var bothTrue = bShow && bOnce;
var bothFalse = !bShow && !bOnce;
if (bothTrue || bothFalse)

Now, we just have to figure out how do we need to set button.show and bOnce values:

  • The button.show is true if bothTrue
  • The bOnce is true if bothFalse

5. Let's put all this together

void Update()
{
    handleInputEvents();

    var bothTrue = bShow && bOnce;
    var bothFalse = !bShow && !bOnce;
    if (bothTrue || bothFalse)
    {         
        button.show(bothTrue);
        bOnce = bothFalse;
    } 
}

I hope it helped you a bit. :)


UPDATE #1

As it was mentioned by user16217248 the condition can be simplified. If either both are true or both are false, that means their values are the same (if (bShow == bOnce)). Because booleans could be either true or false.

If both of the variables are declared as bool? or Nullable<bool> then the shortened condition might not be suitable because in that case the condition would be evaluated as true if both are null. Which may or may not be the right solution depending on your requirements.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ Yes definitely this looks far more professional than what I was able to cook, much grateful. Yet I will wait for atleast a few days before acceptance. \$\endgroup\$ Sep 23, 2023 at 14:30
  • \$\begingroup\$ We can probably put bothTrue and bothFalse variables as part of the class, so as to possibly reduce the allocation and deallocation process everytime the Update() is called. I am not sure though if local variables in C# behave this way, after being built to different platforms, for e.g WASM for web, Windows binary for Windows and so on. \$\endgroup\$ Sep 23, 2023 at 14:36
  • 4
    \$\begingroup\$ Nice work, although I'm not a big fan of the !a && !b syntax, I'd rather !(a || b) by way of de Morgan's Laws. \$\endgroup\$
    – phyrfox
    Sep 23, 2023 at 16:10
  • 11
    \$\begingroup\$ var bothTrue = bShow && bOnce; var bothFalse = !bShow && !bOnce; if (bothTrue || bothFalse) Why not just if (bShow == bOnce)? \$\endgroup\$
    – CPlus
    Sep 23, 2023 at 16:56
  • 3
    \$\begingroup\$ I tried to focus on the refactoring process rather than how to write it in the most concise form. I showed step-by-step how get started with the refactoring BUT as it was pointed out by others this can be pushed further. \$\endgroup\$ Sep 23, 2023 at 17:07
7
\$\begingroup\$

Why don't you make button.show() simply do an early exit if the visibility state being passed in is already the same? I'm surprised it doesn't already work this way.

void Button::show(bool visible) {
    if (visible == _currentVisibility) {
        return;
    }
    // complicated CPU intensive logic below
    _currentVisibility = visible;
}

Then you can skip all these variable checks and say:

void Update()
{
    handleInputEvents();
    button.show(bShow);
}

Can't change the Button control? Make a wrapper or use inheritance

class ButtonWrapper : public Button {
    std::optional<bool> _visibility;
    void ShowFast(bool visibility) {
        if (_visibility.hasValue() && *_visibility == visibility) {
            return
        }
        show(visibility);
        *_visibility = visibility;
    }
}

Or

class ButtonWrapper {
    std::optional<bool> _visibility;
    Button* _b;
public:
    ButtonWrapper(Button *b) : _b(b) {}
    void ShowFast(bool visibility) {
        if (_visibility.hasValue() && *_visibility == visibility) {
            return
        }
        _b->show(visibility);
        *_visibility = visibility;
    }
}
\$\endgroup\$
4
  • 3
    \$\begingroup\$ The original code is C#... not C++. Would be nice if the wrapper snippets were C# too. Otherwise, I definitely agree that this encapsulating the optimization is the best way to go. \$\endgroup\$ Sep 24, 2023 at 15:14
  • \$\begingroup\$ DoublePlusGood for renaming variables into the UI domain. Here it is better than my original comment above. ALL refactorings not doing this are just "rearranging the deck chairs on the Titanic" so to speak; more tidy but not addressing the fundamental problem - obscuring understanding of just what is going on \$\endgroup\$
    – radarbob
    Sep 24, 2023 at 19:32
  • \$\begingroup\$ The best answer! Masterfully refactored. Concise thanks to detail encapsulation. Show() local variables instantly clarify just what is going on without requiring a universal variable renaming. Potential side effects are minimized; an example of how Open/Close Principle can be applied. Future variable name improvements are not risking naming collsions. \$\endgroup\$
    – radarbob
    Sep 24, 2023 at 20:01
  • 1
    \$\begingroup\$ ButtonWrapper class is premature. An ill advised suggestion without knowing the larger design context. The premise - "Can't change the Button control?" - seems to disregard the Button class API. Also, pointer notation is foreign to C# coders. Underscore-prefixed-variables is the same sin as type-prefixed-names; it's not better OO if it is C++ idiomatic. \$\endgroup\$
    – radarbob
    Sep 24, 2023 at 20:27
6
\$\begingroup\$

Since this is tagged , rather than setting a flag somewhere else in the script, you can just do the following:

void Update()
{
    if (Input.GetButtonDown("<physical button name>")
        button.show(true);

    if (Input.GetButtonUp("<physical button name>")
        button.show(false);
}

Input.GetButtonDown only returns true the first frame the button is pressed; likewise for Input.GetButtonUp.

A more comprehensive solution might be using the Unity Input System, which is more event driven, but might also be overkill if you don't need the flexibility the system provides.

\$\endgroup\$
4
\$\begingroup\$

You currently have two booleans, capable of representing 4 states (like many other answers have shown with helpful truth tables). But there's actually only 3 cases here: show, hide, and do nothing. If you can change how handleInputEvents works, one nullable boolean should be enough to handle it all:

bool? bShow = null;

void Update() {
    handleInputEvents();

    if (bShow.HasValue) {
        button.show(bShow.Value);
        bShow = null;
    }
}

void handleInputEvents() {
    // Set bShow to true if there's been input to show the button,
    // set it to false if there's been input to hide the button,
    // and otherwise don't set it at all.
}

Unrelated, but I can't help pointing this out:

Unless you need the bShow value elsewhere in your code, I would recommend making handleInputEvents return the data representing the results (just the bool?, or a struct or a tuple if it resolves multiple things), instead of assigning to a field and reading it later. That way the dependencies are clear – your button-showing code depends on input handling.

// For example
struct InputResult {
    public bool? showButton;
    public bool? sayHello;
}

void Update() {
    var result = handleInputEvents();

    if (result.showButton.HasValue) {
        button.show(result.showButton.Value);
    }
}

InputResult handleInputEvents() {
    var result = new InputResult();
    // Populate the struct
    return result;
}

An even better option could be inverting control and using event-driven/reactive programming, where you can get rid of booleans like this altogether. :)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for valuable input, I will checkout event-driven/reactive programming \$\endgroup\$ Sep 26, 2023 at 6:51
2
\$\begingroup\$

I'd do:

void Update()
{
    handleInputEvents();
    if (bShow != bShown) {   // if the display doesn't match reality
        button.show(bShow);  // update the display to match
        bShown = bShow;      // and remember that for future checks
    }
}

(personally, I'm not fond of prefixing variable names with their type, but I take it that is a convention for the technology you are working in?)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ type-prefixing variable names is contrary to OO principles and goals; it is not idiomatic C#. In fact OO nirvana can be said to be when the code is virtually oblivious to type - just ask Alan Kay. Variables should say what they are or are doing, not the implementation details of how. \$\endgroup\$
    – radarbob
    Sep 24, 2023 at 19:37
2
\$\begingroup\$

I think a higher level review of how this whole button thing works.

I want to express a slightly different approach and instead of bShow and bOnce (which are not that clearly defined, especially bOnce) and use physical_button and virtual_button defined as below.

  • physical_button the most recent sampled electrical signal of the real physical button.

  • virtual_button is the state of the displayed button that the user sees.

I'd like to use 3 states: 0, 1 with their usual meaning of off/on and -1 implying unknown. This -1 useful for initialization.

int physical_button = -1;  // or perhaps type signed char for these 2.
int virtual_button = -1;

void UpdateAlt() {
  handleInputEvents();
  // Unless the physical button has been sampled at least once, nothing to do.
  // ... and display deserves to remain in its initial state. 
  if (physical_button >= 0) { // This test useful if physical_button can be reset to -1.
    if (virtual_button != physical_button) {
      virtual_button = physical_button;
      button.show(virtual_button);
    }
  }
}
\$\endgroup\$
3
  • \$\begingroup\$ if (physical_button >= 0) seems unnecessary unless at some point you want to reset physical_button (but not virtual_button) to the "unknown" state \$\endgroup\$
    – Bergi
    Sep 24, 2023 at 5:07
  • \$\begingroup\$ Yes, physical_button is -1 at program start, but so is virtual_button, which means that the virtual_button != physical_button condition makes physical_button >= 0 unnecessary. \$\endgroup\$
    – Bergi
    Sep 24, 2023 at 12:48
  • \$\begingroup\$ @Bergi OK, I agree with your point that if (physical_button >= 0) is needed when "at some point you want to reset physical_button". \$\endgroup\$ Sep 24, 2023 at 14:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.