-1
\$\begingroup\$

First and foremost I want to say that I'm fully aware GOTO should normally be avoided in production code and it tends to be unreadable for others. For this specific case however, the code will not be read by anyone but me and the question is being asked mostly so I can understand what other options do I even have available since I lack experience with C++.

What the following code snippet does is it picks the best object from a queue based on a set amount of parameters. The parameters have different importance and the importance is reflected in the if statement nesting - the most important parameters and conditions are on the outside. The subsequent parameters are only used as tiebrakers in case the more important parameters match.

I also have not been able to find anything regarding this type of use for goto:

void class_method() {

    std::queue<int> my_queue;
    // generation of many objects happens here

    // Now I want to find the best element and I store the values for decider parameters
    // storing the object itself would be very inefficient as I care only about some key values
    int best_element_id = 0;
    int best_parameter_a = 0;
    int best_parameter_b = 0;
    // ...
    int best_parameter_z = 0;

    while (!my_queue.empty()) {
        if (my_queue.front().a_getter() > best_parameter_a) {
            // Found new best element since parameter A is most important
            int best_element_id = my_queue.front().id_getter();
            int best_parameter_a = my_queue.front().a_getter();
            int best_parameter_b = my_queue.front().b_getter();
            // ...
            int best_parameter_z = my_queue.front().z_getter();
        }
        else if (my_queue.front().a_getter() == best_parameter_a) {
            if (my_queue.front().b_getter() > best_parameter_b) {
                // Found new best element since parameter B is second most important
                int best_element_id = my_queue.front().id_getter();
                int best_parameter_a = my_queue.front().a_getter();
                int best_parameter_b = my_queue.front().b_getter();
                // ...
                int best_parameter_z = my_queue.front().z_getter();
            }
            else if (my_queue.front().a_getter() == best_parameter_a) {
                // ...
                // Many more checks
                // ...

                // Till last parameter is checked
                if (my_queue.front().z_getter() > best_parameter_z) {
                    int best_element_id = my_queue.front().id_getter();
                    int best_parameter_a = my_queue.front().a_getter();
                    int best_parameter_b = my_queue.front().b_getter();
                    // ...
                    int best_parameter_z = my_queue.front().z_getter();
                }
            }
        }

        my_queue.pop();
    }
};

The conditions are obviously oversimplified and there are actually less than 26 parameters, I'm using a-z just to generalize the problem.

Now I was obviously very displeased with this code and attempted to come up with some way to have the lines that update the parameters in one place. This is what I came up with:

void class_method() {

    std::queue<int> my_queue;
    // generation of many objects happens here

    // Now I want to find the best element and I store the values for deciding parameters
    // storing the object itself would be very inefficient as I care only about some key values
    int best_element_id = 0;
    int best_parameter_a = 0;
    int best_parameter_b = 0;
    // ...
    int best_parameter_z = 0;

    while (!my_queue.empty()) {
        if (my_queue.front().a_getter() > best_parameter_a) {
            // Found new best element since parameter A is most important
            goto new_best;
        }
        else if (my_queue.front().a_getter() == best_parameter_a) {
            if (my_queue.front().b_getter() > best_parameter_b) {
                // Found new best element since parameter B is second most important
                goto new_best;
            }
            else if (my_queue.front().a_getter() == best_parameter_a) {
                // ...
                // Many more checks
                // ...

                // Till last parameter is checked
                if (my_queue.front().z_getter() > best_parameter_z) {
                    goto new_best;
                }
            }
        }
        
        if (false) {
            new_best:
            int best_element_id = my_queue.front().id_getter();
            int best_parameter_a = my_queue.front().a_getter();
            int best_parameter_b = my_queue.front().b_getter();
            // ...
            int best_parameter_z = my_queue.front().z_getter();
        }
        my_queue.pop();
    }
};

What other options do I have?

Would such usecase be considered okay? If it is, maybe there is a more elegant way to write that without the if (false) condition? I'm also aware that I couldve used the GOTO to jump back to the first condition evaluating to True but I'm not a big fan of jumping backwards in code so I assumed putting the condition if (false) at the end makes this somewhat more readable...

Last but not least, my IDE throws warnings because of the if (false) line, but I'm assuming the compiler will not optimize it away, right?

\$\endgroup\$
12
  • 1
    \$\begingroup\$ you missed something important, what does your code do? Currently, it looks very hypothetical \$\endgroup\$
    – user228914
    Commented Nov 18, 2020 at 6:08
  • 1
    \$\begingroup\$ The thing is, Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site. all the ... in your question indicate that. \$\endgroup\$
    – user228914
    Commented Nov 18, 2020 at 6:26
  • 2
    \$\begingroup\$ The question is still very unclear, no idea what to code does, where the function belongs, whether it is a part of a class. Nothing at all :/ \$\endgroup\$
    – user228914
    Commented Nov 18, 2020 at 6:41
  • 2
    \$\begingroup\$ Also read this" If your question contains stub code, then there are significant pieces of the core functionality missing, and we need you to fill in the details. Excerpts of large projects are fine, but if you have omitted too much, then reviewers are left imagining how your program works. " \$\endgroup\$
    – user228914
    Commented Nov 18, 2020 at 7:02
  • 3
    \$\begingroup\$ As hypothetical code, this does look off topic. Are you aware of std::priority_queue? \$\endgroup\$
    – user673679
    Commented Nov 18, 2020 at 7:43

2 Answers 2

4
\$\begingroup\$

GOTO should normally be avoided in production code and it tends to be unreadable for others

Readability isn’t just “for others”, it’s for you, as well.

That said, if a goto makes code more easily readable, go for it. It’s just that these situations are exceedingly rare in C++. And your code isn’t one of them. In fact, your goto solution, where the label is nested in an if (false), is primarily cryptic.

As it stands, the control flow in your code is obfuscated. This isn’t just due to the goto, but the goto certainly doesn’t help (and in fact obfuscating control flow is precisely the quality that makes goto so controversial).

Your code also contains errors, which are hard to spot. For instance, you test my_queue.front().a_getter() == best_parameter_a twice. The second time is almost certainly intended to test b_getter() == best_parameter_b. Such errors get more likely with complex, deeply nested code flow.

It’s hard to give concrete recommendations without concrete code. Fundamentally, the purpose of your goto seems to be to skip a loop iteration when some conditions are not met. You’d usually use continue for this. Except, ah, you have that extra unconditional queue.pop() at the very end.

One straightforward solution is to replace if (false) with if (update), and replace the gotos with update = true;. That makes the intent immediately more explicit, and thus the code more readable.

But I would be tempted to rewrite the logic completely, and in such a way that we can use continue inside a for loop here. The rewrite isn’t straightforward, since it requires not only inverting all your conditions inside the loop, but also restructuring the flow of the conditional statements.

for (; not my_queue.empty(); my_queue.pop()) {
    if (my_queue.front().a_getter() < best_parameter_a) {
        continue;
    }
    if (my_queue.front().a_getter() == best_parameter_a) {
        if (my_queue.front().b_getter() < best_parameter_b) {
            continue;
        }
        if (my_queue.front().a_getter() == best_parameter_a) {
            // ...
            // Many more checks
            // ...

            // Till last parameter is checked
            if (my_queue.front().z_getter() <= best_parameter_z) {
                continue;
            }
        }
    }
    
    int best_element_id = my_queue.front().id_getter();
    int best_parameter_a = my_queue.front().a_getter();
    int best_parameter_b = my_queue.front().b_getter();
    // ...
    int best_parameter_z = my_queue.front().z_getter();
}

But this is still deeply nested and, as mentioned, that makes it unreadable and error-prone. The fact that you’re testing a large number of hard-coded parameters is a rancid code smell. When squinting, the whole code looks like it’s trying to establish a lexicographical between two vectors of parameters. So your code should really express that, using appropriate data structures and operator overloading:

while (not my_queue.empty()) {
    if (best_parameter < my_queue.front()) {
        best_parameter = my_queue.front();
    }
    my_queue.pop();
}

There.

This code isn’t (just) superior because it doesn’t require goto. It’s superior because it’s 90% shorter, has no deep nesting, and directly expresses the intent.

\$\endgroup\$
1
  • \$\begingroup\$ Ah, very insightful, the fact that I wrote my_queue.front().a_getter() == best_parameter_a twice was indeed an error. The for loop solution is very neat, I did not consider using for loops like that. The solution with appropriate data structures and operator overloading definetly beats everything else on readability though. \$\endgroup\$ Commented Nov 18, 2020 at 10:54
1
\$\begingroup\$

In my personal opinion, using GOTO is totally acceptable. But regarding your example, your code looks not very optimized in the first place.

Nevertheless, some tips for you when using GOTO.

You should use labels in the most possible upper scope (if possible, function scope). Labels should be written in capital letters. You should never jump over any declaration of any kind. Write a label at the same column like the function scope brackets, to enhance readability. Don't use arbitrary constructs like if(false), if you are using GOTO anyway, you might as well use it also in its full glory.

For example:

while (!my_queue.empty()) {
    int best_element_id;
    int best_parameter_a;
    int best_parameter_b;
    // ...
    int best_parameter_z;
    
    if (my_queue.front().a_getter() > best_parameter_a) {
        // Found new best element since parameter A is most important
        goto NEW_BEST;
    }
    else if (my_queue.front().a_getter() == best_parameter_a) {
        if (my_queue.front().b_getter() > best_parameter_b) {
            // Found new best element since parameter B is second most important
            goto NEW_BEST;
        }
        else if (my_queue.front().a_getter() == best_parameter_a) {
            // ...
            // Many more checks
            // ...

            // Till last parameter is checked
            if (my_queue.front().z_getter() > best_parameter_z) {
                goto NEW_BEST;
            }
        }
    }
    goto NO_NEW_BEST;
    
NEW_BEST:
    best_element_id = my_queue.front().id_getter();
    best_parameter_a = my_queue.front().a_getter();
    best_parameter_b = my_queue.front().b_getter();
    // ...
    best_parameter_z = my_queue.front().z_getter();
    
NO_NEW_BEST:
    my_queue.pop();
}
\$\endgroup\$
2
  • \$\begingroup\$ Ah, I knew there had to be a cleaner way instead of using `if (false)"... Aside from the code not being optimized in the first place, your code does look a lot more readable. \$\endgroup\$ Commented Nov 18, 2020 at 11:00
  • \$\begingroup\$ By the way, to ensure my statement, "You should use labels in the most possible upper scope (if possible, function scope).", I would write the while-loop as a GOTO-loop. Because using a GOTO-statement within a while-loop hurts a structured text while-loop ^^. What you are actually looking for is an interleaving loop (An interleaving loop consists out of 2 or more loops, looping into each other.), a tool which structured texts doesn't provides, the only way is a GOTO. GOTO is even the most precise way to accomplish this task, any other method is just a work around. \$\endgroup\$
    – paladin
    Commented Nov 18, 2020 at 11:19

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