51

Not an actual problem but I'm looking for a pattern to improve the following logic:

void PrintToGameMasters()
{
    std::string message = GetComplicatedDebugMessage(); // This will create a big string with various info
    for (Player* player : GetAllPlayers())
       if (player->IsGameMaster())
           player->SendMessage(message);
}

This code works, but the issue I have is that in most cases there aren't any gamemasters players so the message composition will be done for nothing.

I'd like to write something that would only create the message on the first use of that variable, but I can't come up with a good solution here.

EDIT: To make this question more precise, I'm looking for a solution that's not specific to strings, it could be a type without a function to test if it's initialized. Also big bonus points if we can keep the call to GetComplicatedDebugMessage at the top of the loop, I think a solution involving a wrapper would solve this.

closed as primarily opinion-based by Peter Duniho, Rob, Zoe Aug 18 at 11:50

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 6
    "Also big bonus point if we can keep the call to GetComplicatedDebugMessage at the top of the loop" - why? Then you are right back to initializing the message even if there are no game masters in the list. If it really is so complicated to create the message, why not do so the first time SendMessage(message) is actually called? – Remy Lebeau Aug 15 at 16:02
  • 3
    The general technique is called "laziness" or "lazy initialization", and is quite common in functional languages. – pjc50 Aug 16 at 15:38
  • Will the message always be the same, or does it have to be recomputed each time the function is called? – Barmar Aug 16 at 19:10
  • As a note, this task is significantly easier if the object returned by GetAllPlayers() provides a means of testing whether it contains any game masters. That would let you shortcut your logic, so that the function only does anything if there's at least one value where player->IsGameMaster() is true. auto& players = GetAllPlayers(); if (!players.hasGM()) { return; } – Justin Time Aug 16 at 20:09
  • @JustinTime It would maybe make more sense to simply create a GetAllGameMasters function instead. – Burak Aug 20 at 15:56

13 Answers 13

78

Whereas std::string has empty value which might mean "not computed", you might use more generally std::optional which handle empty string and non default constructible types:

void PrintToGameMasters()
{
    std::optional<std::string> message;

    for (Player* player : GetAllPlayers()) {
       if (player->IsGameMaster()) {
           if (!message) {
              message = GetComplicatedDebugMessage();
           }
           player->SendMessage(*message);
       }
    }
}
  • 5
    This is good. It removes the reliance on "empty == not calculated yet" with absolutely no source code lines overhead. 👍 In fact, sod it, this is what I'd do. – Lightness Races in Orbit Aug 15 at 22:40
  • 9
    This answer has the best balance of clarity, efficiency, and extensibility so far. – ApproachingDarknessFish Aug 15 at 23:31
  • 4
    Or less pretty. See how that's a matter of opinion? – Lightness Races in Orbit Aug 16 at 13:52
  • And my opinion (we know what those are like) is that there is great value in having { and } vertically aligned with nothing between them (as opposed to having to hunt for the match through all the lines above. – WGroleau Aug 16 at 17:09
  • 2
    @ruohola Coding standards are highly subjective and the general guideline around here is to just ignore any compulsion one might have to comment on or change that (when they're following commonly-used coding standards such as those in this answer, at least). – Dukeling Aug 16 at 22:55
38

Use data-oriented design; keep two lists of players: game masters and non-game masters (or all players like you have now + a separate vector of pointers to game-masters only).

void PrintToGameMasters()
{
    auto players = GetGameMasters(); // Returns ONLY game master players
    if (players.begin() != players.end()) {
        std::string message = GetComplicatedDebugMessage();
        for (Player* player : players) {
            player->SendMessage(message);
        }
    }
}

The goal is to minimize if-statements inside loops.

Optimize for the most common case, not the most generic one; the most common case is that a player is not a game master; so avoid looping over them.


P.S. Since you're developing a game, I want to add this link to Mike Acton's cppcon talk which you might find interesting.

  • I like this a lot. – Lightness Races in Orbit Aug 15 at 22:39
  • 9
    Even better would be if (players.begin() == players.end()) { return; } and then you can do the actual work without an extra indentation block. – scohe001 Aug 16 at 15:25
  • 1
    Also likely to be faster because, presumably, you're likely to have a lot less game masters than regular players, which means the list would be expected to be noticeably smaller. – code_dredd Aug 17 at 3:52
  • 1
    I don't think this is "data-oriented design", but whatever name you give it, "keep two lists" stinks. You've lost all concept of ordering between master and non-master players, and increased the complexity of iterating the combined set of players in every other place that needs it. – Ben Voigt Aug 18 at 4:28
  • 1
    @BenVoigt It’s fine DOD. Ideally you wouldn’t actually keep two lists, GetGameMasters just returns a view on the total list with a filter interposed. Pre ranges C++ doesn’t make this easy so it’s not a common pattern, but it isn’t hard either. – Konrad Rudolph Aug 19 at 9:32
36

Some good ideas here, but I like to keep it a bit more simple:

void PrintToGameMasters()
{
    std::string message;

    for (Player* player : GetAllPlayers())
    {
       if (player->IsGameMaster())
       {
           if (message.empty())
              message = GetComplicatedDebugMessage();

           player->SendMessage(message);
       }
    }
}

Everybody can follow this, and it's cheap as chips… plus it's easy as pie to debug.

  • 4
    Probably suitable for this case. But in general, what if message happens to be empty but still expensive to calculate? – sebrockm Aug 15 at 14:14
  • 4
    @sebrockm Then you can introduce a bool hasMessage instead. Either way it's pretty trivial logic. – Lightness Races in Orbit Aug 15 at 14:15
  • I do know it's trivial :) but then the entire question is trivial, yet OP asked it. So maybe it's not trivial for them :) – sebrockm Aug 15 at 14:21
  • 3
    This is both short and simple. + vote for recognizing when not to use "cool" stuff – user1316208 Aug 15 at 14:45
  • 1
    I mean I kinda get the instinct. But IME for something like this, you're more likely to just make it more complicated than it needs to be. Maybe that's just me though :) – Lightness Races in Orbit Aug 15 at 22:38
30

You can use std::call_once with a lambda to call the function the first time you find a game master like

void PrintToGameMasters()
{
    std::once_flag of;
    std::string message;
    for (Player* player : GetAllPlayers())
       if (player->IsGameMaster())
       {
           std::call_once(of, [&](){ message = GetComplicatedDebugMessage(); });
           player->SendMessage(message);
       }
}
  • 1
    I like this a lot - good use of the C++ Standard Library. Have an upvote! – Bathsheba Aug 15 at 14:03
  • 32
    I like it too, but this mechanism is specifically designed to handle the problem of concurrency (e.g.: making sure multiple threads only call something once). As in...it comes with the overhead of an interlock (which could still be very small overhead, but non-zero cost). Whereas a cheap alternative to call_once that was not designed to handle concurrency would simply be a bool where the once_flag is, and the construct of if(!hasBeenCalled) { hasBeenCalled=true; fn(); } <-- That's all you need. But std::call_once is too convenient to pass up. Just be aware about what it's for. – Wyck Aug 15 at 14:12
  • 2
    @Wyck Totaly agree. I just wanted to show it off. Lightness Races in Orbit's answer is the best for efficiency. – NathanOliver Aug 15 at 14:13
  • 2
    Nice! That's the kind of answer I was hoping for. It seems it doesn't fit as a generic solution to always reuse because of the lock overhead, but it's very nice to know. – Kelno Aug 15 at 15:24
13

Wrap the message in a mutable lambda:

auto makeMessage = [message = std::string()]() mutable -> std::string&
{
    if (message.empty()) {
        message = GetComplicatedDebugMessage();
    }
    return message;
};

for (Player* player : GetAllPlayers())
   if (player->IsGameMaster())
       player->SendMessage(makeMessage());
  • I like this one especially, I can make some more generic wrapper out of this easily. – Kelno Aug 15 at 15:19
4

Not sure if this is the best pattern, but you can delay computation with a lambda:

void PrintToGameMasters()
{
    std::string message = "";
    auto getDebugMessage = [&message]() -> const std::string& { 
        if (message.empty()) {
            message = GetComplicatedDebugMessage();
        }
        return message;
    };

    for (Player* player : GetAllPlayers())
       if (player->IsGameMaster())
           player->SendMessage(getDebugMessage());
}
4

You can extend the approach of using an std::optional (as in Jarod41's answer) with lazy evaluation on top. This would also fulfill the requirement to "keep the call to GetComplicatedDebugMessage at the top of the loop".

template <typename T>
class Lazy : public std::optional<T> {
public:
    Lazy(std::function<T()> f) : fn(f) { }
    T operator*() {
        if (!*this)
            std::optional<T>::operator=(fn());
        return this->value();
    }
private:
    std::function<T()> fn;
};

void PrintToGameMasters()
{
    Lazy<std::string> message(GetComplicatedDebugMessage);
    for (Player* player : GetAllPlayers())
        if (player->IsGameMaster())
            player->SendMessage(*message);
}
2

I'm not sure why you want to keep the definition of message above the loop. If someone's reading the code to analyze what happens in the case where std::end(GetAllPlayers())==std::begin(GetAllPlayers)(), you don't want to clutter their mental workspace with irrelevant variables.

If you're willing to give that up, then static is your friend:

void PrintToGameMasters()
{
    for (auto const &player : GetAllPlayers())
        if (player->IsGameMaster())
        {
            //Initialization of a static variable occurs exactly once, even when multithreaded,
            //precisely when the defining line is hit for the first time
            static auto const &message{GetComplicatedDebugMessage()};
            player->SendMessage(message);
        }
}
  • I imagine the definition was just above the loop to prevent it from getting calculated multiple times (in the absence of another way to do this). If it were me, I'd drop that part from the answer and simply say something like "If GetComplicatedDebugMessage is stateless (it's return value doesn't change over multiple calls of the method) and you want to call this only once over all calls of PrintToGameMasters, you can use static: {code}. If the method is not stateless, you can use one of the solutions suggested in other answers". – Dukeling Aug 17 at 0:20
1

This works. As the MIT license would put it:

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED

#include <Windows.h>
#include <cstdlib>
#include <cstdio>
#include <string>

struct Player {
    bool isGameMaster;
    int id;
};

int __stdcall IsGameMaster(Player* self) {
    return self->isGameMaster ? 1 : 0;
}

// Could've been "SendMessage"... but Windows.h
void __stdcall SendMessageToPlayer(Player* self, std::string* msg) {
    printf("Player %d says: %s\n", self->id - 1000 + 1, msg->c_str());
}

Player g_players[18];
Player* __stdcall GetAllPlayers(void){
    return &g_players[0];
}

std::string le_message = "hi, I'm a game master";
std::string* __stdcall GetComplicatedMessage(void) {
    puts("GENERATING COMPLICATED MESSAGE. HOGGING CPU FOR 3 DAYS!");
    return &le_message; // to make my assembly life easier
}

__declspec(naked) void PrintToGameMasters(void){
    __asm {
        push ebp;
        mov ebp, esp;
        sub esp, 8;

        call GetAllPlayers;
        mov [ebp-4], eax;

        // this is 'i', the loop iteration counter
        // I chose esi because it is preserved by stdcalls
        xor esi, esi;

        do_loop:

        // Player* player = &g_players[i];
        mov ebx, esi;
        imul ebx, SIZE Player;
        add ebx, [ebp-4]; // ebx = g_players + sizeof(Player) * i, or &g_players[i]

        // if (player->IsGameMaster()) {
        push ebx;
        call IsGameMaster;
        test eax, eax;
        jz no_print;

        // msg = GetComplicatedMessage();
        get_msg_start:
        call GetComplicatedMessage;
        mov [ebp-8], eax;
        jmp short delete_self;
        get_msg_end:

        // player->SendMessage(msg);
        push [ebp-8];
        push ebx;
        call SendMessageToPlayer;

        // }
        no_print:
        inc esi;
        cmp esi, 18;
        jb do_loop;

        mov esp, ebp;
        pop ebp;
        ret;

        delete_self:
        mov ecx, get_msg_start;
        mov eax, get_msg_end;

        sub eax, ecx;
        mov byte ptr [ecx], 0xEB; // jmp short
        mov byte ptr [ecx+1], al; // relative offset
        jmp get_msg_end;
    }
}

int main(){
    for (int i = 0; i < 18; i++) {
        g_players[i].isGameMaster = (i == 12 || i == 15); // players 13 and 16
        g_players[i].id = i + 1000;
    }

    DWORD oldProtect;
    VirtualProtect(&PrintToGameMasters, 0x1000, PAGE_EXECUTE_READWRITE, &oldProtect);

    PrintToGameMasters();

    return 0;
}

code working as intended

It is much faster than the if (!message) message = GetMessage() approach, unless you have a CPU with a branch predictor (which you likely do). In that case, it's slower (or maybe equally fast, but not faster), uglier, less portable, and will likely get you killed by a psychopath.

1

This is literally one of the things std::future is designed to solve:

void PrintToGameMasters()
{
    auto message = std::async(
        std::launch::deferred,
        []{return GetComplicatedDebugMessage();}
    );
    for (Player* player : GetAllPlayers())
       if (player->IsGameMaster())
           player->SendMessage(message.get());
}

Invoking std::async with std::launch::deferred causes the task to be “executed on the calling thread the first time its result is requested”.

1

You can use a custom local type with a conversion operator:

void PrintToGameMasters()
{
    struct {
        operator std::string const &(void)
        {
            static auto const real_value{GetComplicatedDebugMessage()};
            return real_value;
        }
    } message;
    for (auto const &player : GetAllPlayers())
       if (player->IsGameMaster())
           player->SendMessage(message);
}

Of course, this assumes that GetComplicatedDebugMessage is in fact stateless. Otherwise, you'll need to resort to capturing a lambda, or similar tricks described in the other answers here.

  • The idea is good but your answer contains several errors: Your code will recompute the value every time, it won’t cache it. You probably forgot a static in front of the local real_value variable. The code, as it stands, also causes UB because the lifetime extension for const& won’t extend beyond return statements (i.e. it can’t leave the scope). Just remove the reference on the local variable. – Konrad Rudolph Aug 19 at 9:14
  • Yes; your corrections are precisely what I meant. I've corrected the answer now. – Jacob Manaker Aug 20 at 6:47
-1

Really hope this helps

Try maybe implementing this logic:

#include <iostream>

using namespace std;

int main()
{
    bool GameMaster,first_time,used_before;

    GameMaster = true;
    first_time = false;
    used_before = false;

    GameMaster = first_time;
    first_time = used_before;


    for( int i = 0; i < 5; i++ ) {

        if(GameMaster==used_before) {
            cout<<"    First Time";
            GameMaster = true;
        }

        if(GameMaster!=used_before) {
            cout<<"    Old News";
        }
    }

    return 0;
}

With Response:

  First Time    Old News    Old News    Old News    Old News    Old News 
  • FWIW i = i + 1 can be replaced by ++i, if(GameMaster!=used_before) can be replaced by else, You can also get rid of two of the three bool's as you only need one flag to know if one thing has been done. – NathanOliver Aug 15 at 14:34
  • I didn't say im expert, I was trying to pass variable values, without affecting the previous one. I used this logic before for button polling and it did the job, I didn't consider that bc he only needs to read the change once, 3 variables are overkill. Looks like it could have been done easier, thank you for your suggestions. – Andrei Elekes Aug 15 at 21:24
  • 1
    Dunno, seems too wordy. Also fix your indentation!! – Lightness Races in Orbit Aug 15 at 22:39
-1

static variables are initialized on first time through. So:

void PrintToGameMasters()
{
    for (Player* player : GetAllPlayers())
       if (player->IsGameMaster()) {
           static std::string message = GetComplicatedDebugMessage(); // This will create a big string with various info
           player->SendMessage(message);
       }
}

This is, of course, assuming that calculating the value once at most (rather than once per call at most) is a valid way of proceeding. It's not clear from how you phrased your task whether this is the case.

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