Problems with GoTo

I've coded in C++ since a few months ago and I often found people who recommended me not to use the much hated Goto...


But I haven't found any reason yet!


I know that sometimes it makes you lose your own code logic, but look at this:

Code:

---

#include <iostream>//        std::cin        std::cout
#include <string>//        getline()        tolower()
#include <limits>//        numeric_limits<streamsize>::max()
using namespace std;


void EnterNumber()
{
        int something;
cin1:
        cout << "Enter something: ";
        cin >> something;
        if (!cin)
        {
                cin.clear();
                cin.ignore(numeric_limits<streamsize>::max(), '\n');
                cout << "invalid answer!\n\n";
                goto cin1;
        }
}

---

Yes, I can get the same result with a do/while cycle, but I think this is more understandable, shorter and maybe a bit more performing too!


Can you tell me why shouldn't I use GoTo for a function like that?

The problem with a do while loop in this case is that you'll likely have to move the reading and error handling to a function that is called in the loop condition, or resort to using a flag that's somewhat clumsy.

On the other hand, the problem with the goto is that it's too powerful, so it doesn't convey intent quite as well as a loop control structure.

There is, however, the option of a controlled infinite loop:

Code:

---

void EnterNumber()
{
    int something;
    for (;;)
    {
        cout << "Enter something: ";
        if (cin >> something)
        {
            break;
        }
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        cout << "invalid answer!\n\n";
    }
}

---

In practice, instead of a break you might want to return the value read and hence just return once the read is successful.

EDIT:
Come to think of it though, because your reading isn't complex, you might be able to get by with a simple while loop:

Code:

---

void EnterNumber()
{
    int something;
    while ((cout << "Enter something: ") && !(cin >> something))
    {
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
        cout << "invalid answer!\n\n";
    }
}

---

On the other hand, it's probably better to read a line as a string with getline and then parse the string for the int value, in which case my earlier example would likely be more convenient (and you'll have less error handling).

Thank you very much!

I couldn't figure out how to implement a loop without using a boolean variable... So problem solved ;)

Concluding, when should or shouldn't I use GoTo? Are there some cases in which it is better than a cycle?

Generally, you shouldn't use goto in C++. For your flow of control needs, you should reach for a loop control structure first, possibly with break or continue or a well named boolean flag.

Because C++ doesn't provide a version of break that allows breaking out from nested loops at once, it can be reasonable to consider goto for that... but see if well named boolean flags would still be readable, or maybe it would be best to recognise that you have too much nesting in that piece of code so you should move some part of it to a new function.

If you have been introduced to C first, you might be aware of an idiomatic error handling+cleanup strategy in C that uses goto. However, in C++, exceptions along with binding resources to object lifetimes tend to be the norm.

Also here -> Problems with GoTo - C++ Forum

well, bugger the goto. I'm more worried about stuff like

Code:

---

#include <iostream>//    std::cin    std::cout
#include <string>//    getline()    tolower()

---

Your comments are going to become awfully long (and even more useless) if you insist on commenting why you're including a header

On a historical note, goto is seen as "wrong" because in the 60's Dijkstra (a famous professor) published an article saying so. Since then, everyone adopted this opinion as cannon.
Ok... abusing the use of goto can make your code confusing, but there is nothing wrong to use goto occasionally...

Thanks all for the precious support!

I will implement your suggentions for sure

Which is better among these?

Code:

---

unsigned short method1(const string& text = "", unsigned short max = -1){
    string ins;
    unsigned short val;

    while (true)
    {
        cout << text;
        getline(cin, ins);


        try
        {
            val = stoi(ins);
            if (val > max) throw val;
        }
        catch (...)
        {
            cout << "Invalid answer!\n\n";
            continue;
        }
       
        break;
    }


    return val;
}

---

Code:

---

unsigned short method2(const string& text = "", unsigned short max = -1){
    unsigned short val;

    while (true)
    {
        cout << text;
        if (cin >> val && val <= max) break;

        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');

        cout << "Invalid answer!\n\n";

    }


    return val;
}

---

The results are the same, but I'm going to use this function many times so I want to be sure that I'm using the best performing one :o

Quote:

---

Originally Posted by Foxel

The results are the same

---

They don't do the same thing: reading with getline and parsing the string read means that the entire line is handled: a subsequent read starts on a fresh line. Reading with the overloaded operator>> leaves whatever is not parsed during the read on the rest of the line, waiting to be read next. Furthermore, you want to read and store an unsigned short, but stoi parses for an int, not an unsigned short.

If the former is what you want, then seeing that you have additional validation to do, moving the reading and parsing into another function may make more sense after all:

Code:

---

bool readParseAnswer(unsigned short& answer, const std::string& prompt)
{
    using namespace std;

    cout << prompt;
    string line;
    if (!getline(cin, line))
    {
        return false;
    }

    int result;
    try
    {
        result = stoi(line);
    }
    catch (const std::exception& e)
    {
        return false;
    }

    if (result < 0 || static_cast<unsigned int>(result) > numeric_limits<unsigned short>::max())
    {
        return false;
    }

    answer = static_cast<unsigned short>(result);
    return true;
}

unsigned short readAnswer(const std::string& prompt = "")
{
    unsigned short answer;
    while (!readParseAnswer(answer, prompt))
    {
        std::cout << "Invalid answer!\n\n";
    }
    return answer;
}

---

Quote:

---

Originally Posted by laserlight

They don't do the same thing: reading with getline and parsing the string read means that the entire line is handled: a subsequent read starts on a fresh line. Reading with the overloaded operator >> leaves whatever is not parsed during the read on the rest of the line, waiting to be read next.

---

It doesn't, because I cleanup the rest of the line, with

Code:

---

cin.ignore(numeric_limits<streamsize>::max(), '\n')

---

so I think I'll use the second method which works without any further casts or controls, doesn't it?

Quote:

---

Originally Posted by Foxel

It doesn't, because I cleanup the rest of the line, with

---

No, you only do that on a read error.

Quote:

---

Originally Posted by laserlight

No, you only do that on a read error.

---

You're right

Code:

---

unsigned short method2(const string& text = "", unsigned short max = -1){
    unsigned short val;

    while (true)
    {
        cout << text;
        if (cin >> val && val <= max) break;

        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');

        cout << "Invalid answer!\n\n";

    }
    cin.ignore(numeric_limits<streamsize>::max(), '\n');

    return val;
}


---

What about this? It seems better than casting the variable each time...