7
\$\begingroup\$

I'm developing programs on Golang. But now you have to study at the university and have the following task.

Calculate the continued fraction using the function indicators. The number of fractional elements must be specified from the keyboard. 1 + 1 / (1 + 1 / (1 + 1 / (1 + 1 / ....))). The number of elements of the fraction must be at least 5. Consider three cases, for example, 5, 7 and 10.

Enter the number of elements of the continued fraction: 6 
Solution - 1.61538

Please check my code.

#include <iostream>
using namespace std;
int main(){

    int n;
    double t;
    t=2;
    bool exit;


    while (exit != true) {
        printf("To exit the programs, type 0\n");
        cout << "Enter the number of continued fractions: " << endl;
        cin >> n;

        if (n == 0) {
            exit = true;
        } else {
            if (n < 5) {
                printf("The number of elements of a fraction must be at least 5");
                cin.get();
            } else {
                for (int a = 1; a < n; a++) {
                    t = 1 / t + 1;
                }
                cout << "Decision - " << t << endl;
            }
        }
    }
    printf("Exiting the program");
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Are negative numbers to be considered? In that case the algorithm does not work as expected with that t+1, if it's not needed you should check that negative numbers can't be inserted. \$\endgroup\$ – Mattia Righetti May 26 '18 at 8:25
  • \$\begingroup\$ No, negative numbers are not considered. Check for negative numbers will do. Thank you \$\endgroup\$ – Евгений Гусев May 26 '18 at 8:28
  • 2
    \$\begingroup\$ @ЕвгенийГусев Why are you mixing printf() and std::cout? \$\endgroup\$ – πάντα ῥεῖ May 26 '18 at 8:29
  • \$\begingroup\$ Golang's habit use printf. So you do not need to do this? \$\endgroup\$ – Евгений Гусев May 26 '18 at 8:31
  • 1
    \$\begingroup\$ Have you tried entering the same number twice and looked at the output? Do you get the intended behaviour? \$\endgroup\$ – Carsten S May 26 '18 at 20:40
11
\$\begingroup\$
\$\endgroup\$
  • 1
    \$\begingroup\$ Why not use while (true) instead of for (;;)? It is a lot more readable. \$\endgroup\$ – Tyilo May 26 '18 at 22:51
  • \$\begingroup\$ An explicit return 0; in main isn't bad style, but it would be more interesting and justified if any other code-paths returned non-zero for error conditions. \$\endgroup\$ – Peter Cordes May 26 '18 at 23:44
  • 2
    \$\begingroup\$ @Tyilo I agree. But for (;;) may be more idiomatic for some people (C-style, perhaps?). And for example Visual Studio's compiler emits a 'condition always true' warning for while(true) but not for for (;;). \$\endgroup\$ – Pablo H May 27 '18 at 17:32
  • \$\begingroup\$ @PabloH: Classic C style (before #include <stdbool.h>) is while(1){} or while(42){}. I'd only use for(;;){} for code-golf (shortest number of source bytes), not for readable style, but maybe I'd use it if I was using a compiler that warned for infinite-loop idioms with while. (I don't normally use MSVC. I tried it on the Godbolt compiler explorer for while(true), and even with -Ox -Wall (which warns for every standard-header function that wasn't inlined!), I don't get a warning for a while(true) loop: godbolt.org/g/aVqat5) \$\endgroup\$ – Peter Cordes May 28 '18 at 11:25
  • 1
    \$\begingroup\$ It's mostly personal preference. For what it's worth have a look at this SO question which lists the different loops. \$\endgroup\$ – yuri May 28 '18 at 11:33
10
\$\begingroup\$

Bug: double t=2 is only initialized once, outside the outer loop, so every time you enter a new number, you're doing that many more iterations to further refine your approximation to the Golden Ratio. Putting double t=2; closer to where it's used, instead of the top of the function, is good style as well as fixing that.

Bug: bool exit; and then while(!exit) reads an uninitialized value from exit. This is C++ Undefined Behaviour (because its address is never taken). gcc and clang (with optimization enabled) decide to assume that it was initially true, so they compile your main accordingly: they just call printf("Exiting the program") and return, without even generating asm code for the loop. clang6.0 warns about it with -Wall (<source>:9:10: warning: variable 'exit' is used uninitialized whenever function 'main' is called [-Wsometimes-uninitialized]), but gcc8.1 doesn't warn :( See them both on the Godbolt compiler explorer. If your code happened to work on your C++ implementation, you got lucky.

Potential bug: EOF on input leads to n being used uninitialized: If extraction fails, n is left unmodified (before C++11). You'll have an indeterminate value if the user hits control-d on the first prompt, or redirects from /dev/null. Or, before C++11, even if they type xyz or some other non-digits. (Not Undefined Behaviour, because cin::operator>> takes its arg by reference, so n must have an address.) Use int n = 0;, or the more modern C++-only initializer syntax int n{0}, so your program exits in the EOF case, as well as the invalid-input case. (Non-digit input produces a value of 0; only EOF or I/O errors leave the value unmodified.) Set n=0 inside the loop so reading a valid n and then EOF doesn't result in an infinite loop.


using namespace std is frowned upon, but if you don't like writing std::everything you can import the names of the things you do actually use. using std::cout; at global scope allows you to use cout << "foo" instead of `std::cout << "foo".

But if you import some things but not others from std::, you'll have source lines with std::cin next to lines with cout, and that inconsistency will be distracting to future readers (and you'll have to remember which names you imported). It might not be too messy if you import everything you use from a category of thing, e.g. using std::cin, std::cout, std::cerr;, or using std::min, std::max;. (A comma-separated list for using requires C++17). That might look ok even when you mix std:: with imported stuff on one line, e.g. I/O manipulators: cout << std::hex << var << std::dec;.

It's probably best to just use std:: everywhere, because you'll have an easier time making a modified copy of your code for another project without having to adjust it for a different set of using std::stuff you've imported in a different project.


Don't use printf or endl, and use 1 function call instead of 2.

You can print a multi-line string with a single cout << instead of making two separate function calls to std::cout::operator<<. (And using printf for one line but cout for the other makes no sense, see other answers). In C and C++, two string literals separated by whitespace (including newlines in the source) act as a single string literal. So you can still format your source nicely whether printing or assigning to a const char *prompt:

std::cout << "line 1\n"
             "prompt: ";

// exactly equivalent to this ugly mess:
std::cout << "line 1\nprompt: ";

You can omit the newline from the end of your prompt if you like: reading cin flushes cout. stdout is line-buffered by default (when it's connected to a terminal / tty), so normally cout won't send data to the OS until it sees a \n newline, but the flush triggered by cin << n will make sure the prompt appears.

Don't use endl; just end your string with a \n. Or cout << var << '\n'; if you weren't printing a string literal. (Or "\n", but printing a single character should be at least as efficient as printing a string of length 1.)

std::endl explicitly flushes your output stream. You almost never want this. cout will be full-buffered if your program's output was redirected to a file or pipe, and forcing flushes with endl defeats full buffering and makes your program less efficient, using more system calls to get the same data written.

Your exit message omitted a newline, so the first prompt printed by the shell will show up at the end of that line. Programs are expected to have their output end with a newline. You really don't need this message at all; it's basically a debug logging message and isn't useful for users of the program. You can already tell when a program exits because your shell prints a new prompt, or something else happens depending on how you ran your program.

"Decision - 1.61803" looks confusingly like a negative. Decision is not the best choice of English word for this result: there was no decision-making, just the specified number of iterations. A better result format might be cout << "n=" << n << " result: " << t << "\n\n";. Two newlines separate the output into blocks of input->result nicely.


Write error messages to cerr: that's what it's for. Maybe someone will use your program from a script like echo {4..9} | ./cont_frac > /dev/null. They'll still see the error message about 4 being less than 5 on stderr, even though they redirected the prompts (on stdout) to /dev/null.

(For programs where you expect non-interactive use, command line options are nice. But "driving" programs that only have prompts does happen in phylogenetics, for example, where many programs are written by biologists, not programmers, and only have interactive menus / prompts. So people using the program from a script just feed it input on stdin. As long as the program creates an output file so you don't have to parse the output to separate data from prompts, it's ugly and brittle, but not horrible.)

cin.get() seems to be useless there: In the normal case (where the user types 4 and hits return), cin.get() returns the newline. (I checked by using int tmp = cin.get(); and outputting the integer value of the ASCII code.) I don't think it's useful for the user to be able to type 4x return and have cin.get eat the x instead of causing n=0 in the next outer-loop iteration. Just remove it.


Your loop structure is a bit tricky to implement nicely.

What you really want to semantically express is "while the user has entered a non-zero value". while(true){} with an if(!n) break; after the I/O is pretty good, and pretty easy to understand. (You definitely don't want an exit flag.)

The point of this section of my answer is to explore the alternatives, and see if they're more or less readable.

You can write an ugly while loop that does express exactly the semantics you want:

  // string literals separated only by whitespace are concatenated at compile time.
  const char* prompt =  "To exit the programs, type 0\n"
                        "Enter the number of continued fractions: ";
  int n;
  // reading cin flushes cout, so it's ok that the prompt doesn't end with a \n
  while(cout << prompt, cin >> n, n != 0) {
      ...
  }

The value of the comma operator is its last operand, so you can put separate expressions before the one the while() statement will check. But this is definitely unusual, and will make the code less easy to read than break.

Perhaps a helper function or lambda would be better?

static inline int prompt_n()
{
    cout <<  "To exit the programs, type 0\n"
             "Enter the number of continued fractions: ";
    // reading cin flushes cout
    int n = 0;  // if extraction fails, value is left unmodified
    cin >> n;
    return n;
}

We can define this block right inside main (before the loop) using a lamba (C++11 or later for auto to be usable this way):

#include <iostream>
using std::cin, std::cout, std::cerr;  // C++17 required for comma-separated list

int main()
{
    auto prompt_n = [] {
        cout <<  "To exit the programs, type 0\n"
                 "Enter the number of continued fractions: ";
                    // reading cin flushes cout
        int n = 0;  // On EOF, value is left unmodified (with libstdc++ at least)
        cin >> n;
        return n;
    };

    while (int n = prompt_n()) {
        if (n < 5) {
            cerr << "The number of elements of a fraction must be at least 5\n";
        } else {
            double t = 2;
            for (int a = 1; a < n; a++) {
                t = 1 / t + 1;
            }
            cout << "result for n=" << n << ": \t" << t << "\n\n";
        }
    }

    return 0;
}

(Godbolt compiler explorer: compiles with no warnings as C++17.)

Sample output:

$ g++ -std=gnu++17 -O2 -g -Wall cont_frac_lambda.cpp
$ ./a.out
To exit the programs, type 0
Enter the number of continued fractions: 5
result for n=5:         1.625

To exit the programs, type 0
Enter the number of continued fractions: 7
result for n=7:         1.61905

To exit the programs, type 0
Enter the number of continued fractions: 5
result for n=5:         1.625

To exit the programs, type 0
Enter the number of continued fractions: peter@volta:/tmp$

Notice that 5 got the same result both times. And that hitting control-d to send an EOF exited cleanly.


Declaring int n inside the while() condition makes it in-scope for the loop body, might be considered bad style. It won't work for any comparison other than the implicit non-zero test, and worse, while(int n = foo() != 0) {} compiles, giving you a 0 or 1 initializer from converting the boolean foo() != 0 to integer. Declaring int n; outside the loop would avoid that problem, and lets you put parens around the assignment within a condition to remind human readers that it's an assignment, not ==.

                  // avoiding while(int n...)
    int n;
    while ((n = prompt_n()) != 0) {
        ...
    }

(An assignment inside a while condition is a common idiom for reading until EOF, or looping over characters in a C string with while (c = *p++) { ... }. At least it's a common idiom in C; I'm not sure about modern C++.)


If you're weird like me and thinking about how the compiler will compile your function into asm, you might have noticed that the loop ends by printing a string literal, and starts by printing another string literal.

We could tighten up the loop's asm by skewing the loop so the first prompt is done before entering, and then prompting for the next n is done at the bottom of the loop. That lets us combine the newlines after the result into the prompt. (And with pointer math, we can use the same const char *prompt variable for the first prompt, too, just skip the first 2 newline bytes to get the regular prompt. So the program doesn't need 2 almost-identical copies of the prompt. Compilers aren't smart enough to merge string literals that share a suffix, only actually-identical literals, last I checked, so unfortunately gcc and clang would miss this optimization if you didn't do it manually.)

This is silly because we're just calling I/O functions, so smaller code-size for the whole function is probably best. But this was fun. See main_silly_optimized() in the Godbolt link for a whole main() that does this. The changed parts are:

int main_silly_optimized()
{
          // prompt function takes an arg now
    auto prompt_n = [](const char *prompt) {
        cout <<  prompt;
        int n = 0;  // On EOF, value is left unmodified (with libstdc++ at least)
        cin >> n;
        return n;
    };

    const char *prompt = "\n\nTo exit the programs, type 0\n"
                 "Enter the number of continued fractions: ";
    int n = prompt_n(prompt+2); // without the leading newlines
    while (n) {
        if (n < 5) {
            cerr << "The number of elements of a fraction must be at least 5\n";
        } else {
            double t = 2;
            for (int a = 1; a < n; a++) {
                t = 1 / t + 1;
            }
            cout << "result for n=" << n << ": \t" << t; // << "\n\n";
            // 2 newlines printed as part of the prompt
        }
        n = prompt_n(prompt);
    };

    return 0;
}

This is really not bad for readability.

You could bring n = prompt_n(prompt); inside the two branches of the if(), and for example use prompt+2 or prompt+1 in the error case, or a different prompt altogether.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good work to spot that bug regarding the initialization of t. I've adopted that into my refactoring. Same for the potential bug about the uninitialized n. \$\endgroup\$ – πάντα ῥεῖ May 28 '18 at 17:43
  • \$\begingroup\$ @πάνταῥεῖ: IIRC, I spotted the n bug from looking at compiler output of the original on Godbolt, before looking at re-structuring it: the loop optimized away! (with optimization enabled, of course). BTW, any thoughts on using std::cout, etc.;? Does anyone do that in real codebases, if they want to avoid using namespace std? I mostly look at optimizing code for speed, so code I touch typically ends up full of comments about why this or that is the most efficient way to do something, and how it will compile for x86. But the std:: thing is orthogonal to performance. \$\endgroup\$ – Peter Cordes May 28 '18 at 21:42
  • \$\begingroup\$ I don't believe that std:: can be really blamed to be orthogonal for performance in general. \$\endgroup\$ – πάντα ῥεῖ May 28 '18 at 21:51
  • \$\begingroup\$ @πάνταῥεῖ: I meant that using std::vector; vs. writing std::vector<int32_t> won't ever change the asm output of your code, so that style issue never affects performance. I was trying to say that I normally care more about performance than style, and sometimes you need to bend your source to hand-hold the compiler into seeing optimizations. (And do{}while() is often good because that's the natural loop idiom for asm: Why are loops always compiled into "do...while" style (tail jump)?). But good style is nice when it doesn't hurt performance. \$\endgroup\$ – Peter Cordes May 28 '18 at 21:54
5
\$\begingroup\$

Some minor points to improve:

  • Declare variables at the scope in which they're used. In other words, declare n directly before the cin. The same goes for t.
  • You don't need the exit variable. Simply break out of the loop.
\$\endgroup\$
  • \$\begingroup\$ > you don't need the exit variable. Simply break out of the loop. did not quite understand you, you can read more? \$\endgroup\$ – Евгений Гусев May 26 '18 at 8:43
  • 1
    \$\begingroup\$ Worth mentioning that declaring / initializing t inside the outer loop actually fixes a bug (that each new user input is supposed to start with a fresh t=2, according to the wording of the prompts). \$\endgroup\$ – Peter Cordes May 28 '18 at 11:28
4
\$\begingroup\$

To make your code more readable you should simplify and restructure it a bit like follows:

    int n = 0; // initialize to prevent an endless loop if EOF is received with `cin` 

    do {
        double t = 2;
        cout << "To exit the programs, type 0\n";
        cout << "Enter the number of continued fractions: ";
        cin >> n;

        if (n != 0) {
            if (n < 5) {
                cout << "The number of elements of a fraction must be at least 5\n";
                continue;
            }

            for (int a = 1; a < n; a++) {
               t = 1 / t + 1;
            }
            cout << "Decision - " << t << endl;
        }
    } while (n != 0);
    cout << "Exiting the program\n";
  • t is initialized inline without an extra statement, also that's done inside the loop (eliminating a bug) because it's a variable never needed outside.
  • the unnecessary exit boolean variable was eliminated by transforming the original while() loop to a do {} while(); loop
  • the else clauses are eliminated by changing the condition to n != 0, and using continue to restart the loop in case of not accepted input for n

As you see the refactoring shortens your code significantly without changing the behavior. In general readability of code is enhanced if it can be read straight forward, else clauses often require you have to look back at the introducing if() condition, and will break you in order to follow the sequential flow of statements.

\$\endgroup\$
  • \$\begingroup\$ I think you're missing a << in the first cout. Also, you don't need two separate function calls to cout, you can cout << "first line\n" and on the next line "prompt: "; and let the string literals concatenate at compile time. \$\endgroup\$ – Peter Cordes May 26 '18 at 23:47
  • \$\begingroup\$ I posted an answer with a while(int n = prompt()) {} loop structure that expresses the semantic meaning you want: while the user has input a non-zero number. do{}while() is great for loops that always run at least once, but here you had to repeat the n != 0 condition in two places to get the right semantics. \$\endgroup\$ – Peter Cordes May 27 '18 at 10:41
  • \$\begingroup\$ TBH this has to be taken much further! "What does this function do?" "It prints to the screen AND accepts input AND contains business logic AND terminates the program". One "and" is already too much. \$\endgroup\$ – Vorac May 29 '18 at 8:35

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.