10
\$\begingroup\$

I'm trying to learn exception handling in C++.

I wanted to read two integers and divide them and print them.

The code should throw an exception when the second integer is zero, ask the user to re-enter the second integer, and then complete the divide operation.

I coded something like this:

#include <iostream>
#include <stdexcept>

using std::cin;
using std::cout;
using std::endl;

int main() {
    int i1, i2;
    cout << "Give two integers" << endl;
    cin >> i1;
    try {
        first:
        cin >> i2;
        if (i2 == 0)
            throw 0;
        else
            cout << i1 / i2 << endl;
    }
    catch (...) {
        cout << "Second integer cannot be zero , please enter a valid integer" << endl;
        goto first;
    }
    system("pause");
    return 0;
}

However, I was advised not to use goto at all costs.

Can anyone give me some alternative way?

\$\endgroup\$
  • 1
    \$\begingroup\$ Regardless of whether you should or shouldn't use gotos as a question of policy, or coding habits - you often can't use gotos, or rather a goto might mess up your program. I'm not sure if that's the case for going-to from the catch to the try block, but honestly - if in doubt, I just wouldn't try it. You might find what you're "learning" is undefined compiler-specific and platform-specific behavior. \$\endgroup\$ – einpoklum - reinstate Monica Feb 12 '17 at 22:12
  • 4
    \$\begingroup\$ @einpoklum Good catch: The standard says A goto or switchstatement shall not be used to transfer control into a try block or into a handler.. So this program exhibits undefined behaviour. \$\endgroup\$ – Felix Dombek Feb 12 '17 at 23:34
  • 1
    \$\begingroup\$ @FelixDombek: Actually it just won't compile. \$\endgroup\$ – einpoklum - reinstate Monica Feb 12 '17 at 23:45
  • \$\begingroup\$ @einpoklum ah well. I was in Code Review mode :) \$\endgroup\$ – Felix Dombek Feb 12 '17 at 23:49
  • 1
    \$\begingroup\$ @FelixDombek: I don't actually know the standard at all. If something doesn't seem to have reasonable semantics, not even one of several options, then I assume that: (1) it doesn't compiler or (2) it's an esoteric clause in the standards which I really shouldn't bank upon anyway or (3) it's something important for me to learn. That's by order of frequency of past experience :-) \$\endgroup\$ – einpoklum - reinstate Monica Feb 13 '17 at 0:04
26
\$\begingroup\$

Exceptions are for exceptional cases

I'm trying to learn exception handling in C++.

This is a very bad example to learn exceptions. They are meant for situations where you cannot know that there is something wrong. For example if you want to use std::vector<int> but you're out of memory:

std::vector<int> vec(4000000000LL); // will likely throw std::bad_alloc

I'll add an exercise for you at the end of this review. But let's have a look at your code.

A review of your code

You're not using using namespace std, which is a great plus.

However, you include <stdexcept>, which isn't necessary here. You don't use any of the standard exception, instead, you throw 0. However, system is in <cstdlib>. You should include that one instead.

Next, naming.

int i1, i2;

Those names don't have any meaning. What is i1? What is i2? Naming is hard, but it's important. You're going to use them as numerator and denominator, or as dividend and divisor, so call them appropriately:

int numerator, denominator;

Getting rid of goto

Now to your try block. Don't. Use. goto. You want to repeat the block until you didn't get 0. We can do that with a simple while:

int main (){
    int numerator, denominator;

    cout << "Give two integers" << endl;
    cin >> numerator; 

    // continue forever                               (1)
    while(true) {
        try {
            cin >> denominator;
            if (denominator == 0)
                throw 0; // this will "go to" catch   (2)

            cout << numerator / denominator << endl;
            break; // we didn't throw, we can stop    (3)
        }
        catch (...) {
            cout << "Second integer cannot be zero , please enter a valid integer" << endl;
        }
    }

    system("pause");
    return 0;
}

A "clean" version without exceptions

However, that's not how I would write the program, since exceptions are meant for exceptional cases. I would write

#include <iostream>
#include <cstdlib>

int main() {
    int numerator, denominator;

    std::cout << "Please enter two integers." << std::endl;
    std::cin >> numerator >> denominator;

    while(denominator == 0) {
        std::cout << "Second integer cannot be zero. Try again." << std::endl;
        std::cin >> denominator;
    }

    std::cout << (numerator / denominator) << std::endl;

    system("pause");
    return 0;
}

As you can see, exceptions aren't really necessary to keep the user from entering a zero. Furthermore, the control flow is a lot easier to read.

Note that system("pause"); won't work on Linux, since there is no application called pause. If possible, try to use your IDEs features to keep your console window alive even after your program finished, or have the output logged somewhere, or use it in the CMD/PowerShell.

Exception exercise

We need a better exercise for your exceptions. How about this?

int divide(int numerator, int denominator) {
    if(denominator == 0)
        throw "divide: division by zero";
    return numerator / denominator;
}

This is a valid place for an exception. A function has only a single return value, so it can only tell you that something is wrong with an exception*.

Exercise: Try to use that function in your code and handle the exception. Also show the exception to the user, they're probably interested in what went wrong.

You should use the code similar to

std::cout << divide(numerator, denominator) << std::endl;

that is you don't check the denominator. Note that this is still a contrived example.

* Technically, that's not true. You can use references, pointers, or wrap the return value in some struct/variant, but let's keep things simple

\$\endgroup\$
  • \$\begingroup\$ boost::optional :) \$\endgroup\$ – Matthieu M. Feb 12 '17 at 19:29
  • \$\begingroup\$ @MatthieuM. I'm aware ;). That's what I meant with "wrap the return value in some struct". Is there something like Haskell's Either String Int in boost? boost::optional<int> is more like Maybe Int. \$\endgroup\$ – Zeta Feb 12 '17 at 19:37
  • \$\begingroup\$ What is system("pause")? Sounds non-portable to me \$\endgroup\$ – Hagen von Eitzen Feb 12 '17 at 21:58
  • 1
    \$\begingroup\$ @HagenvonEitzen system() is a function from the C library, that calls the system's command processor to execute the specified command. pause is an MS-DOS command, that's available in DOS and the Windows NT command shell; Linux doesn't have a direct equivalent, indeed making system("pause") non-portable. \$\endgroup\$ – Justin Time 2 Reinstate Monica Feb 12 '17 at 22:57
  • \$\begingroup\$ @Zeta there is boost.variant and boost.any. I believe that variant matches the description better. \$\endgroup\$ – Incomputable Feb 13 '17 at 4:23
7
\$\begingroup\$

Lots of bad practices exhibited here…

First of all, you probably shouldn't be using exception handling here at all, as Incomputable already pointed out in a comment. Exceptions should be for truly exceptional situations. If you expect a zero input as a possibility and are going to explicitly handle it, then I would argue that it is not exceptional at all. Thus, a simple while loop and conditional test would suffice: just keep looping until the user enters a valid input.

Second, your variable names leave a lot to be desired. What are i1 and i2? It's okay to use terse, single-letter variable names like i when their purpose is obvious, as for a loop index, but that's not the case here. Therefore, you should use descriptive variable names. Since we're doing division, maybe dividend and divisor would be good choices?

Third, system("pause") is just terrible and should never be used in real code. I guess you're using this to work around the fact that your development environment closes the console environment as soon as the program finishes execution, instead of leaving the window on the screen so you can see its output. Don't use code as a hitch to work around your IDE's bad behavior—learn to configure your IDE so it does what you want. If you absolutely must rely on such a hitch, use cin.get(). But there's really no "good" way to do this, so prefer not doing it.

Fourth, if you're going to throw exceptions:

  • Never throw integers, or string literals, or anything else other than an exception object as an exception. You should either throw one of the standard exceptions provided by the standard library (e.g., std::runtime_error), or you should derive your own exception class from one of these standard exceptions and throw an instance of that class. In this case, you'd be fine using std::domain_error.

  • Don't use catch (...) unless you actually want to catch all possible exceptions, which you hardly ever want to do. You are effectively forced into doing this because you're throwing an integer, but since you're not going to be doing that anymore, you can properly catch an instance of an exception class by const reference: catch (const std::domain_error& ex).

  • Don't use goto as a crutch for proper flow control. Part of the reason you were having trouble writing sensible code here is because you were trying to cram all of your logic into a single function. You had code to retrieve input from the user, code to carry out the program logic (the division operation), and code to print the result all in the same function! Break it up into logical pieces. You can often get away with this in a toy program, but it is a bad habit to get into.

Here is the exception-based code:

int Divide(int dividend, int divisor)
{
    if (divisor == 0)
    {
        throw std::domain_error("Attempted to divide by zero.");
    }
    return (dividend / divisor);
}

int main()
{
    int dividend;
    int divisor;
    std::cout << "Input two integers: " << std::endl;
    std::cin >> dividend;
    while (true)
    {
        std::cin >> divisor;
        try
        {
            std::cout << Divide(dividend, divisor) << std::endl;
            break;  // division succeeded, so stop looping
        }
        catch (std::domain_error& ex)
        {
            std::cout << "The second integer cannot be zero; please enter a valid integer: " << std::endl;
        }
    }
    return 0;   // you can omit this for the main function
}

And here is the simpler code that doesn't use exceptions. Notice that it is exactly the same logic as before, explicitly detecting the error and handling it, but it doesn't use exceptions for flow control—just normal flow control structures like loops (which we're using anyway!):

int main()
{
    int dividend;
    int divisor;
    std::cout << "Input two integers: " << std::endl;
    std::cin  >> dividend >> divisor;
    while (divisor == 0)
    {
        std::cout << "The second integer cannot be zero; please enter a valid integer: " << std::endl;
        std::cin  >> divisor;
    }
    std::cout << (dividend / divisor) << std::endl;
    return 0;   // you can omit this for the main function
}

While what you've done to avoid using namespace std; is good, I prefer to always explicitly std::-qualify all references. It's not like it entails an excessive amount of typing.

\$\endgroup\$
  • \$\begingroup\$ You've just destroyed my exercise :[. \$\endgroup\$ – Zeta Feb 12 '17 at 18:13
  • 1
    \$\begingroup\$ Oops, sorry @Zeta. I just finished reading your answer and noticed the same thing. Oh well, have an upvote. :-) \$\endgroup\$ – Cody Gray Feb 12 '17 at 18:14
  • 1
    \$\begingroup\$ Ah, it's fine. Also, your review adds several things my one misses, e.g. system, catch(...). \$\endgroup\$ – Zeta Feb 12 '17 at 19:03
  • \$\begingroup\$ @Zeta I thank you for the patience you put into typing all this. I'm going to follow every single thing you said. \$\endgroup\$ – Accidental Genius Feb 13 '17 at 20:01
3
\$\begingroup\$

As an addition to the points made in other answers, note that your program doesn't compile, since in C++ you cannot jump into a try block...

g++ -std=c++14 -O2 -Wall -pedantic -pthread main.cpp && ./a.out
main.cpp: In function 'int main()':
main.cpp:13:9: error: jump to label 'first'
         first:
         ^~~~~
main.cpp:22:14: note:   from here
         goto first;
              ^~~~~
main.cpp:22:14: note:   enters try block

... and, indeed, you also shouldn't be able to jump into a try block.

\$\endgroup\$
  • \$\begingroup\$ The question was "Can anyone give me some alternative way?". I don't feel this is an answer. \$\endgroup\$ – Dawood says reinstate Monica Feb 13 '17 at 11:42
  • \$\begingroup\$ @DavidWallace: But the site is "code review". I reviewed OP's code - or rather, added an element of the review missing from other answers. Also, see edit. \$\endgroup\$ – einpoklum - reinstate Monica Feb 13 '17 at 11:51
  • \$\begingroup\$ @einpoklum I'm a novice but, the code compiled in a jiffy in my visual studio community ide. \$\endgroup\$ – Accidental Genius Feb 13 '17 at 19:58
  • \$\begingroup\$ Your "v"? Did you mean Visual Studio? I wouldn't trust that... if they have some extension of the language which supports this, you're putting yourself in compiler-specific territory, and I don't believe that's where you wanted to be. Your code is not valid C++. \$\endgroup\$ – einpoklum - reinstate Monica Feb 13 '17 at 20:01
  • \$\begingroup\$ Weird. The compiler bundled with Visual Studio 2015 (v19) compiles this with no errors. Earlier versions, including VS 2010 and 2013, generate the expected error: "C2705: illegal jump into 'try' scope". The error is still documented for VS 2015, but I'm not seeing it generated. Not sure why. \$\endgroup\$ – Cody Gray Feb 14 '17 at 5:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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