Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question
5  
Exceptions are for exceptional cases. User trying to (intentionally or not) break the program is not exceptional, thus simple while loop should suffice. – Incomputable 11 hours ago
1  
You should not need goto in C++ code. It is useful in certain types of C functions, but the vast majority of C programmers are unlikely to encounter those cases (e.g. OS kernel development). – Snowman 8 hours ago
    
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. – einpoklum 6 hours ago
    
@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. – Felix Dombek 5 hours ago
    
@FelixDombek: Actually it just won't compile. – einpoklum 4 hours ago

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.

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

share|improve this answer
    
boost::optional :) – Matthieu M. 9 hours ago
    
@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. – Zeta 9 hours ago
    
What is system("pause")? Sounds non-portable to me – Hagen von Eitzen 6 hours ago
    
@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. – Justin Time 5 hours ago
    
@Zeta there is boost.variant and boost.any. I believe that variant matches the description better. – Incomputable 14 mins ago

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.

share|improve this answer
    
You've just destroyed my exercise :[. – Zeta 10 hours ago
    
Oops, sorry @Zeta. I just finished reading your answer and noticed the same thing. Oh well, have an upvote. :-) – Cody Gray 10 hours ago
    
Ah, it's fine. Also, your review adds several things my one misses, e.g. system, catch(...). – Zeta 9 hours ago

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.

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.