Do a thing with a derived string for a number of strings

Question

I feel this should be possible more concisely, less code repetition, with a loop and a two-dimensional array or something. But not sure how exactly. Any best practice for review?

// Activate shortcuts for editor menu items like Ctrl+S for "Save Item" etc.
// Deactivate instead with optional "false" - to allow these for keybindings
void dlgTriggerEditor::setShortcuts(const bool active)
{
    QList<QAction*> actionList = toolBar->actions();
    QString actionText;
    // 3/3 save button texts need to be kept in sync
    const QStringList saveButtonNames = {qsl("Save Item"), tr("Save Trigger"), tr("Save Timer"), tr("Save Alias"), tr("Save Script"), tr("Save Button"), tr("Save Key"), tr("Save Variable")};
    for (auto& action : actionList) {
        actionText = action->text();
        if (saveButtonNames.contains(actionText)) {
            action->setShortcut((active) ? tr("Ctrl+S") : QString());
        } else if (actionText == tr("Save Profile")) {
            action->setShortcut((active) ? tr("Ctrl+Shift+S") : QString());
        }
    }
    actionList = toolBar2->actions();
    QString actionText;
    for (auto& action : actionList) {
        actionText = action->text();
        if (actionText == tr("Triggers")) {
            action->setShortcut((active) ? tr("Ctrl+1") : QString());
        } else if (actionText == tr("Aliases")) {
            action->setShortcut((active) ? tr("Ctrl+2") : QString());
        } else if (actionText == tr("Scripts")) {
            action->setShortcut((active) ? tr("Ctrl+3") : QString());
        } else if (actionText == tr("Timers")) {
            action->setShortcut((active) ? tr("Ctrl+4") : QString());
        } else if (actionText == tr("Keys")) {
            action->setShortcut((active) ? tr("Ctrl+5") : QString());
        } else if (actionText == tr("Variables")) {
            action->setShortcut((active) ? tr("Ctrl+6") : QString());
        } else if (actionText == tr("Buttons")) {
            action->setShortcut((active) ? tr("Ctrl+7") : QString());
        } else if (actionText == tr("Errors")) {
            action->setShortcut((active) ? tr("Ctrl+8") : QString());
        } else if (actionText == tr("Statistics")) {
            action->setShortcut((active) ? tr("Ctrl+9") : QString());
        } else if (actionText == tr("Debug")) {
            action->setShortcut((active) ? tr("Ctrl+0") : QString());
        }
    }
}

Here is a python mock-up of the bottom (not yet a) loop for comparison:

texts_and_actions = (
  ("Statistics", "Ctrl+9"),
  ("Debug", "Ctrl+0")
)
for text, action in texts_and_actions:
  if actionText == text:
    setShortcut(action)

Answer 1

Just try implementing your ideas

I feel this should be possible more concisely, less code repetition, with a loop and a two-dimensional array or something. But not sure how exactly.

You have the right idea. Now you have to find a way to turn it into C++ code. Even if you can't imagine the final code from the start, just start writing your idea and work on it until it works. Let's start with your Python code:

texts_and_actions = (
  ("Statistics", "Ctrl+9"),
  ("Debug", "Ctrl+0")
)

You can transform that into an array in C++. Of course, in C++ everything has to have a well-defined type. You could make an array of std::pair here, but even better is to create a struct that holds a text and an action:

struct TextAction {
    std::string text;
    std::string action;
};

And then create an array out of that:

TextAction texts_and_actions[] = {
    {"Statistics", "Ctrl+9"},
    {"Debug", "Ctrl+0"},
};

Notice how it's not so different from the Python code. Then we have your for-loop in Python:

for text, action in texts_and_actions:
  if actionText == text:
    setShortcut(action)

You can use the same in C++:

for (auto& [text, action]: texts_and_actions) {
    if (actionText == text) {
        something->setShortcut(action);
    }
}

There are some issues with this (that are also in your example Python code), which we'll see below.

Naming things

Take a bit more time to give proper names to things. For example, in your question, "action" is used for two things: objects in actionList, and the name of the shortcut in texts_and_actions. Maybe the latter should be named "shortcut" instead? The word "text" is also very generic and can apply to anything that is a string, so you probably want to choose something more specific. Maybe "name", "label" or "description" are better choices?

Make looking up shortcuts faster

Going through the list of texts_and_actions each time to find the right element is going to be slow. In Python you would use a dict here, so you can write:

shortcuts = {
  "Statistics": "Ctrl+9",
  "Debug": "Ctrl+0",
}
…
if actionText in shortcuts:
    setShortcut(shortcuts[actionText])

In C++ you can do something similar by using a std::unordered_map:

std::unordered_map<std::string, std::string> shortcuts = {
    {"Statistics", "Ctrl+9"},
    {"Debug", "Ctrl+0"},
};
…
if (auto it = shortcuts.find(actionText); it != shortcuts.end()) {
    action->setShortcut(it->second);
}

Also, if active is false, you are going to set the shortcut to the empty string regardless of the actionText, so why bother doing a lookup at all? You can just put something like this at the start of each for-loop:

if (!active) {
    action->setShortcut(QString());
    continnue;
}

Avoid repeating yourself

If you find yourself repeating similar code, find ways to avoid it. You already had the idea of using arrays, that is one way. Putting code that is used multiple times into a separate function is also a way. If you have the same list of shortcuts, and want to apply it to several action lists, consider doing something like:

// Overload that sets shortcuts for one action list only
void dlgTriggerEditor::setShortcuts(QList<QAction*> actionList, const bool active)
{
    for (auto& action : actionList) {
        …
    }
}

void dlgTriggerEditor::setShortcuts(const bool active)
{
    setShortcuts(toolBar->actions(), active);
    setShortcuts(toolBar2->actions(), active);
    …
}

It might even be possible to make an array or tuple or references out of toolBar and toolBar2, but for something as simple as this I would not do that.