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

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)

• use a map of actionText (key) to shortcut (value) Jan 26 at 12:59
• Hi @depperm How would I use this map in an example loop? Jan 26 at 13:22
• You need to describe this in more detail than "do a thing". Jan 26 at 13:26
• Hello @Reinderien Which information is missing for you? I don't know how to call the thing, but I showed full working code below. You will see a long if/else construction. Each one checks if actionText has a special value, then continues to call setShortcut with a derived value. There is lots of repetition, and the only differences are the two strings. That is why I want to do the same thing, but only with a loop, and not write the same thing so many times in a row. Jan 26 at 14:20

# 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.

• Thanks for answering both of my questions on this site in such a detailed and helpful manner! Really appreciate it. Rock on! Jan 27 at 9:28
• I successfully implemented most of your suggestions already. Wow! Only when I try to make lookups faster like you explained, I get this error message: error: no matching constructor for initialization of 'std::unordered_map<std::string, std::string>' (aka 'unordered_map<basic_string<char>, basic_string<char>>') Why is it happening? I did not find explanations when searching the web either. Jan 27 at 13:41
• Perhaps you made a small mistake? The code I've shown above compiles without problems. Jan 27 at 14:01
• Thanks a lot for the compiling example! This made me realize I forgot the #include..! Jan 29 at 13:59