Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I understand that using goto in C++ code is strictly unadvised, but sometimes, it really reduces the number of lines of code like in the following case.

This is my code for SPOJ. I know this does not reduce too many lines of code, but in a big project, it potentially could.

#include<iostream>
#include<cstring>
#include<cstdio>
#include<cmath>
using namespace std;
bool debug=false;
typedef long long int lld;
typedef unsigned long long int llu;
int main(int argc , char **argv)
{
    if(argc>1 && strcmp(argv[1],"DEBUG")==0) debug=true;
    lld n,val;
    long double sq;
    while(true){
        scanf("%lld",&n);
        if(n==-1)break;
        n -= 1;
        if(n%3!=0)goto exit;
        n/=3;
        sq=4*n+1;sq=sqrt(sq);
        if(sq-(int)sq!=0)goto exit;
        n=sq;
        if(n%2!=1)goto exit;
        printf("Y\n");
        continue;
        exit:
            printf("N\n");
            continue;
    }
    return 0;
}

What should I do in such situations? Is there a way to do this by making a function call?

share|improve this question
1  
It's not a valid C program as written, so I've removed the c tag as well as mentions in the title and body. –  200_success May 25 at 6:24
1  
See also: Centered Hexagonal Number –  Simon André Forsberg May 25 at 11:37
    
A different abstraction level at which to think about this question is this: how do I set up the control flow graph such that a given point (e.g. exit:) can be gotten to from multiple other points? Off the top of my head, I can name if, return, break (in loops), continue, and break (in switch) which does this, plus of course goto. Since your branching pattern doesn't lend itself to a switch, using if or return seems most appropriate, which is exactly what @200_success and gnasher729 do in their answers. Also: you need multiple indentations for if, not for return. –  Jonas Kölker May 26 at 8:30
    
In most c++ code, seeing goto should mean "refactoring required". While it does reduce the lines of code, it is a sign that a function tries to do too many things (i.e., by using goto, you are probably fixing the wrong problem). –  utnapistim May 28 at 15:43

6 Answers 6

Your use of goto is wholly unjustified, not just because goto is taboo, but because your code has flow-of-control that is hard to follow. Furthermore, the use of goto is not even an effective way to achieve your goal of compactness.


Before addressing the core concern about goto, I'd like to point out that there is a lot of junk in your code:

  • Your compiler should have warned you:

    beehive.cpp:12:11: warning: unused variable 'val' [-Wunused-variable]
        lld n,val;
              ^
    1 warning generated.
    

    You do compile with warnings enabled, right?

  • #include <iostream> and #include <cstring> are superfluous. (Put the remaining ones in alphabetical order.)

  • using namespace std; is superfluous, and even if you used anything in the std namespace, a blanket import like that is a harmful habit.

  • The debug flag is unused.

  • typedef unsigned long long int llu; is never used. Furthermore, the problem guarantees 1 ≤ n ≤ 109, so a long would be sufficient for n.


As you suspected, a function call would help. Your main() should look like this:

int main() {
    long n;
    while ((1 == scanf("%ld", &n)) && (-1 != n)) {
        puts(is_beehive_number(n) ? "Y" : "N");
    }
    return 0;
}

That provides two very important improvements over your code:

  • You can see at a glance what the purpose and structure of your program are. The clutter within the loop is all gone, replaced by a function whose purpose is obvious. The loop is properly structured — the phony while (true) is replaced by a useful test.

  • There is now proper separation of concerns. main() loops, reads the input cases, and prints the results. Most importantly, the is_beehive_number() is a pure calculation function that accepts a long and returns a bool.

When the calculation code is in its own function rather than embedded in the loop, the flow of control can be expressed so much better!

bool is_beehive_number(long n) {
    if (--n % 3 != 0) return false;
    double sq = sqrt(4 * (n / 3) + 1);
    if (sq != (long)sq) return false;
    n = sq;
    return (n & 1);
}

I don't understand the mathematics behind your code; I just transformed the code mechanically.

share|improve this answer
4  
Just to be utterly pedantic, this does actually use things in the std namespace. For functions imported from <c*> headers, the standard says they will be imported into the std namespace, and may optionally be imported into the global namespace (although I don't know of a compiler that doesn't import them into the global namespace). –  Yuushi May 25 at 8:30
2  
the line if (sq != (long)sq) return false; is still very evil. doubles cannot represent every integer value exactly - in the cases where it is not possible, the casted version might differ in the last bit. instead do the following: long arg = 4*(n/3)+1; long sq = (long)sqrt(arg); if (sq*sq != arg) return false;. in gcc the flag -Wfloat-equal is advisable for average projects and should have warned about this. –  example May 25 at 16:00
    
in fact, it should be long sq = (long)(sqrt(arg)+0.5); in the short code I wrote in the last comment. otherwise results like 2.99999995 will falsely be rounded down –  example May 25 at 16:20
3  
I feel compelled to comment that moving the body of a loop into the loop condition obfuscates code, and is a bad idea when done for no reason beyond dogmatically avoiding break statements. Better to keep the loop conditions to being simply conditional tests rather than containing the actual logic of the loop when possible. –  Hurkyl May 25 at 16:31
1  
@Hurkyl It's not dogmatic; I genuinely believe it's the most elegant way to express the loop. The infinite loop needs to terminate if EOF is reached or if -1 is entered. So I wrote the code to say just that. –  200_success May 25 at 17:27

I'd like to roll up suggestions from several comments.

  • From @Yuushi: The functions in <cmath> and <cstdio> are in the std namespace.

  • From @example: Testing equality of a floating-point number and an integer is a bad idea. The floating-point number might be just a tiny bit less than an integer, and the cast would truncate it to the next smaller integer.

  • From @Simon: Centered Hexagonal Number. The formula for centered hexagonal numbers is:

    $$H_i = 3i\ (i + 1) + 1$$

    (I've adjusted the notation and indexing so that the sequence starts with \$i = 0\$.)

    Therefore, to test whether \$n\$ is a centered hexagonal number, we need to test whether \$\dfrac{n - 1}{3}\$ is of the form \$i\ (i + 1)\$.

/**
 * Test whether n is of the form 3 * i * (i + 1) + 1.
 * See http://en.wikipedia.org/wiki/Centered_hexagonal_number
 */
bool is_beehive_number(long n) {
    if (n <= 0)     return false;
    if (n % 3 != 1) return false;

    long root = std::lround(std::sqrt((n - 1) / 3));
    return n == 3 * root * (root + 1) + 1;
}
share|improve this answer

Issue for goto isn't the reduction of the length of the code, is the chaos it brings along. In big projects tracking that spaghetti code you get as result of using it is hard, making debugging and updating a pain. Other thing you need to have in mind is that compilers are optimized enough to generate decent code, so using a low level structure in a higher level to avoid lengthy code is not a good idea compared to structured and clear code.

share|improve this answer
    
To further this, your control flow is challenging to follow. Indentation is not your enemy, and it sure wouldn't kill you to put some space in your if statements. –  Scüter May 26 at 2:42

I think you could use exit(0) for a standard exit and a different number for a nonstandard exit, but I might be wrong. An example:

if(sq-(int)sq!=0)exitError();


void exitError(){
    printf("N\n");
    exit(0);
}

Alternatively, if you wanted to actually crash:

void exitError(){
    printf("N\n);
    exit(1);
}
share|improve this answer
1  
this is actually much much worse than the original code. Exiting in the middle of the code is worse than a goto. –  Zig Mandel May 25 at 15:10
1  
exit(1) is not a crash. It's a clean exit with return code 1. abort() does "crash" the process. Agreed with Zig though, that's not really cleaner. –  Mat May 25 at 15:33
    
@Zig OK, but why? –  mm865 May 25 at 21:16
    
Ok,sorry for my comment without explanation. I think its worse not because exit could look like a crash, but because it doesnt address the issue of code structure with multiple exit points. You still have multiple exit points. sort of like a "goto" outside of the function. In your case the function name is pretty obvious that it will exit, but if it was named differently or used deep inside code, it could even be missed by a programmer searching for "return" to find possible multiple exit points. –  Zig Mandel May 26 at 2:11
    
@mm865 - normally, you should not use exit in your programs. When you use exit on a code path, client code will not be able to recover gracefully after the failure point, no matter what you do (no printing a message, no saving data, no checking of an error code, or using a default value instead of what your code calculates - that code will simply not be reached). You shouldn't put in a function, code that affects all client code like this. At worst, use std::terminate, but in most cases, you shouldn't use that either (for the same reasons). –  utnapistim May 28 at 9:08

Having typedefs for "long long int" and "long long unsigned int" is awful. Putting all your code into main () is awful. "using namespace std" seems like a good idea, until you figure out that telling the compiler exactly what you mean also tells other programmers exactly what you mean.

The goto statements don't give you any benefits here. Here's the same without goto, in fewer lines, and structured. I've been known to use goto's when there was a real benefit (although the C++ language tends to make it a pain), but in this case, there is no benefit.

Another problem is the square root and avoiding rounding errors. You want to know whether some number x is a square. That's easily done by rounding the square root to an integer, and checking whether the square of that integer is indeed x. If x is not a square, then this test will (correctly) fail - no matter how big the rounding errors are.

If x is a square, then you may perhaps be able to prove that the floating point square root will always be equal to the exact integer value, but rounding it to the nearest integer is guaranteed to give the right result, and then checking whether the number is a square is done by squaring the root, which is an exact integer operation and won't fail. No need to check if the root is odd, because an odd number is never the square of an even number.

for (;;) 
{
    scanf ("%lld", &n);
    if (n==-1) break;

    n -= 1;
    if (n % 3 == 0) 
    {
        n = (n / 3) * 4 + 1;
        long long int root = sqrt ((double) n) + 0.5;
        if (root * root == n)
        {
            printf("Y\n");
            continue;
        }
    }

    printf("N\n");
    continue;
}
share|improve this answer
    
Although this example isn't bad, this quickly becomes problematic (arrowhead anti-pattern) the more nested blocks you have, especially in a "code is 80 columns" environment. –  Hurkyl May 26 at 17:32

For variety, I will suggest that a better choice of labels would make what you actually wrote more clear. (this post will only address the flow control issue)

The label exit is misleading since you really aren't exiting the loop. Yes, you are aborting the rest of the calculation you are doing, but the point is to continue on to report failure. If the label better expressed what you're trying to do, it would be more obvious upon reading the code.

I suggest adding a superfluous, complementary label for the section that reports success, although you may prefer a comment to a superfluous label. It may even help to assert that fall-through cannot happen. Consider the below:

while (true) {
    scanf("%lld",&n);
    if (n==-1) { break; }
    n -= 1;
    if (n % 3 != 0) { goto failure; }
    n /= 3;
    sq = 4*n+1;
    sq = sqrt(sq);
    if (sq - (int)sq != 0) { goto failure; }
    n = sq;
    if (n % 2 != 1) { goto failure; }
    goto success;

    abort(); // All branches should go to a loop ending
success:
    printf("Y\n");
    continue;
failure:
    printf("N\n");
    continue;
}

For yet more variety, rather than moving the calculation out of the loop, you could consider moving the I/O out of the loop:

while (true) {
    scanf("%lld",&n);
    if (n==-1) { break; }
    n -= 1;
    if (n % 3 != 0) {
        report_failure();
        continue;
    }
    n /= 3;
    sq = 4*n+1;
    sq = sqrt(sq);
    if (sq - (int)sq != 0) {
        report_failure();
        continue;
    }
    n = sq;
    if (n % 2 != 1) {
        report_failure();
        continue;
    }
    report_success();
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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