23
\$\begingroup\$

The fourth project, continuing my C++ saga with terrible post names. :P

An approximate value of pi can be calculated using the series given below:

$$ \pi \approx 4 \left[ 1 - \dfrac{1}{3} + \dfrac{1}{5} - \dfrac{1}{7} + \dfrac{1}{9} - \cdots + \dfrac{\left( -1\right)^n}{2n + 1} \right] $$

Write a C++ program to calculate the approximate value of pi using this series. The program takes an input \$ n \$ that determines the number of terms in the approximation of the value of pi and outputs the approximation. Include a loop that allows the user to repeat this calculation for new values \$ n \$ until the user says she or he wants to end the program.

Also, I am required to use a for loop at least once in my code.

pi.cpp:

/**
 * @file pi.cpp
 * @brief Calculates pi for the given number of terms
 * @author syb0rg
 * @date 10/3/14
 */

#include <iostream>
#include <limits>
#include <cctype>
#include <cmath>

/**
 * Makes sure data isn't malicious, and signals user to re-enter proper data if invalid
 */
void getSanitizedDouble(long double &input)
{
    while (!(input = std::cin.peek()) && input != '\n')
    {
        if (std::isalpha(input) || std::isspace(input)) std::cin.ignore(); // ignore alphabetic and space characters from input
    }
    while(!(std::cin >> input) || input < 0)
    {
        std::cin.clear(); // clear the error flag that was set so that future I/O operations will work correctly
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // skips to the next newline
        std::cout << "Invalid input.  Please enter a positive number: ";
    }
}

int main()
{
    long double num = 0.;
    char again = '\0';
    do
    {
        long double pi = 0.;

        // get input for height, re-read input if not a positive number
        std::cout << "Enter the number of terms to approximate π: ";
        getSanitizedDouble(num);

        for(auto i = 0; i < num; i++)
        {
            pi += std::pow(-1, i) / (2 * i + 1);
        }
        pi *= 4;

        std::fprintf(stdout, "Approximated value of π for %Lg terms: %Lg\n", num, pi);

        std::cout << "Run the program again (y/N): ";  // signify n as default with capital letter
        std::cin.get();  // absorb newline character from previous input
        std::cin.get(again);
        again = std::tolower(again);
    } while (again == 'y');
}
\$\endgroup\$
12
\$\begingroup\$

Out parameters in C++

In C++, you should almost never use out parameters (variables taken by reference and used to return a value from a function), you can read this excellent article by Eric Niebler. There are few cases where out parameters can make sense:

  • When you want your return to be fast. And even then, return value optimization and move semantics may still be faster.

  • When you have several output values. For example, you want to assign a value to a function and return whether it succeeded:

    bool assign(int from, int& to)
    {
        if (from != 0)
        {
            to = from;
            return true;
        }
        return false;
    }
    

    But even for this situation, there are better solutions such as boost:optional to return both a value and whether it succeeded or not. And if you need several error values, use exceptions. And if you need to actually return several values, you general want to pack them into a dedicated struct or a std::tuple.

  • Input-output parameters: sometimes, you want to take a parameter, read from it, and then write to it again. That is still a valid use case (but these are not strict out parameters anymore).

In your case, it seems that getSanitizedDouble could simply return the read long double instead of having an out parameter.

Type correctness

You could improve your type correctness:

  • That's also a naming issue: I don't expect a function named getSanaitzeDouble to return a long double, but to return a double.

  • The literal used in the expression long double pi = 0.; is incorrect: 0. is a double. The correct long double literal should be 0.0L. We could also use 0.0l, but the l suffix is really too close to 1 not to be dangerous.

Order your includes

Alphabetical order is something you should always use to order your headers (at least in a logical group of headers). That allows a faster search to check whether some headers has already been included:

#include <cctype>
#include <cmath>
#include <iostream>
#include <limits>
\$\endgroup\$
  • \$\begingroup\$ -1 because of sorting C++ includes. These are not namespaces to be free to play around however you please. If you want to have a structure do a logical grouping (std includes, library X includes, whatever includes) but that should be enough... you shouldn't have to see where does a new include alphabetically fits. And for detecting duplicated includes you have make use of static code analysis tools. \$\endgroup\$ – Memleak Oct 3 '14 at 11:56
  • 1
    \$\begingroup\$ @Memleak I mentioned logical grouping. But inside a logical group, you don't always have an obvious logical structure, and an alphabetical ordering is fine. Honestly, it does not cost any time if you did it from the start (unless you are bad at remembering the alphabet) and it saves you from having to run a static analyzer every time you include something. \$\endgroup\$ – Morwenn Oct 4 '14 at 18:28
9
\$\begingroup\$

Simplify the input parsing

I don't see a purpose for this part:

while (!(input = std::cin.peek()) && input != '\n')
{
    if (std::isalpha(input) || std::isspace(input)) std::cin.ignore();
}

I suppose your intent was to ignore alphabetic characters at the beginning of the input, but it doesn't work for me. And I don't think it's necessary anyway. If I enter blah333 as the input, I wouldn't expect any program to treat that as 333.

As for skipping over spaces, std::cin >> input will work just fine, for 333 you will correctly get just the 333.

Why a long double instead of long long

In the problem description it looks like n is an integer. And in the code there's nothing that would need the number to be a double. So why not use a long long instead?

If you change to long long, remember to rename the method from getSanitizedDouble to getSanitizedNum or something, and change the %Lg in the fprintf to %lld.

Unnecessary initializations

As in your previous program, these initializations are pointless, because you inevitable reassign these variables soon after:

long double num = 0.;
char again = '\0';

As @syb0rg pointed out, initialization on declaration is a known best practice:

Always initialize variables upon their declaration. This is so that I do not accidentally try to access the values within those variables if I had not assigned anything to them beforehand.

Coding style

I would recommend to not add comments at the end of lines after statements, but at them on the previous line. Especially comments that are quite long, forcing the reader to scroll right.

\$\endgroup\$
  • 1
    \$\begingroup\$ Regarding your "Unnecessary initializations", I learned to always initialize variables upon their declaration. This is so that I do not accidentally try to access the values within those variables if I had not assigned anything to them beforehand. \$\endgroup\$ – syb0rg Sep 30 '14 at 18:59
7
\$\begingroup\$

The code is not so much about p, but rather the user interaction. This review concentrates on a math.

num is double, and so I presume could be i. Raising a negative number into a non-integer power may give you a problem:

If base is finite and negative and exp is finite and non-integer, a domain error occurs and a range error may occur.

It is much safer (and faster) to avoid pow altogether and just toggle a factor between 1 and -1.

And of course main does too much. At least you need to extract the actual pi calculations into a function.

\$\endgroup\$
5
\$\begingroup\$

Lets read task again

The program takes an input \$n\$ that determines the number of terms in the approximation of the value of \$\pi\$ and outputs the approximation. Include a loop that allows the user to repeat this calculation for new values \$n\$ until the user says she or he wants to end the program.

In my opinion it means following program flow: repeat geting ascending sequence of input values of \$n\$ (because it is a number of terms in approximation, not number of additional iterations).

Before we start

Let's approve program flow.

  • First we need to determine: will it continue or break main loop.
  • Then we need a function that returns \$\pi_i\$ depending on \$\pi_{i-1}, n_{i-1}, n_i\$, where \$i\$ is current index in input sequence of amount of terms in approximation. It can be expressed in the following way:

$$ \pi_i = \pi_{i-1} + 4 \left[ \dfrac{ (-1)^{n_{i-1}+1} }{ 2n_{i-1}+3 } + \cdots + \dfrac{\left( -1\right)^{n_i}}{2n_i + 1} \right], \pi_{-1} = 0 $$

In other words, it returns incrementation of the function \$\pi\$ at interval \$(n_{i-1}, n_i]\$.

Resulting code

#include <iostream>
#include <algorithm>

double pi_increment(const int &from, const int& to) {
    double result = 0;
    for (int n = from + 1; n <= to; ++n) {
        result += (n & 1 ? -1.0 : 1.0) / (2 * n + 1);
    }
    return 4 * result;
}

inline bool check_choice(const char &choice) {
    return choice != 'n' && choice != 'N';
}

int main(int argc, char *argv[]) {
    int previous_n = -1,
        current_n = -1;
    double pi = 0;
    std::cout.precision(15);
    do {
        std::cout << "Write number of terms: ";
        while (previous_n >= current_n) {
            std::cin >> current_n;
            if (previous_n >= current_n) {
                std::cout << "Number of terms should be greater than "
                          << previous_n << ". Enter new value " << std::endl;
            }
        }

        pi += pi_increment(previous_n, current_n);
        std::cout << "Pi(" << current_n << ") equals to " << pi << std::endl;
        std::swap(previous_n, current_n);

        std::cout << "Would you like to continue (Y/n)? ";
        std::cin.ignore();
    }
    while (check_choice(std::cin.get()));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I think you're reading the requirement wrong: the way the OP did it is what the requirement asks for. \$\endgroup\$ – GreenAsJade Oct 1 '14 at 7:12
  • \$\begingroup\$ @GreenAsJade, well, you can ask TC. \$\endgroup\$ – outoftime Oct 1 '14 at 7:15
2
\$\begingroup\$

Structure

Although you have separated out a function to read user input, your algorithm is in main(), mixed in with the printing and interactive looping. I would put the arithmetic into a function with this signature:

long double compute_pi(unsigned int num_places);

This gives a couple of advantages:

  • You can use different main() implementations (for example, I prefer to specify the number of places as an argument, for reasons I'll explain below).
  • You can drop in different compute_pi() implementations (so you can compare the convergence of this algorithm with others you are interested in).

Note also the type of num_places - it doesn't make sense to specify this as a floating-point number.

If I change num_places to unsigned long, I can make a very simple test program (with a bare minimum of checking):

#include <iostream>
#include <string>

int main(int, char **argv)
{
    while (*++argv) {
        auto n = std::stoul(*argv);
        std::cout << n << " terms: " << compute_pi(n);
    }
}

The advantages of reading n from command line arguments include:

  • It can be scripted easily - e.g. in bash: for i in {1..15}; do ./64297 $i; done to show how the sum converges.
  • The program can be run within time to see how changes to the code affect the execution speed.

Arithmetic

Now to the actual implementation. I can extract your code into a function:

#include <cmath>
long double compute_pi(unsigned long num_places)
{
    long double pi = 0.;
    for (auto i = 0u; i < num_places; i++) {
        pi += std::pow(-1, i) / (2 * i + 1);
    }
    pi *= 4;
    return pi;
}

I changed the initilizer for i to be 0ul, to agree with num_places.

The one thing that stands out here is that expensive std::pow call. Because we are just using it to get 1 or -1, we could

  • examine whether i is even or odd: i % 2 ? -1 : 1, or
  • maintain some state, and swap the sign each time around the loop.

I'll take the second option here:

long double compute_pi(unsigned long num_places)
{
    long double pi = 0.;
    long double numerator = 1;
    for (auto i = 0ul;  i < num_places;  i++) {
        pi += numerator / (2 * i + 1);
        numerator = -numerator;
    }
    pi *= 4;
    return pi;
}

Similarly, instead of multiplying for the denominator, we can start with 1 and add 2 each time around the loop:

long double compute_pi(unsigned long num_places)
{
    long double pi = 0.;
    long double numerator = 1;
    unsigned long denominator = 1;
    for (auto i = 0ul;  i < num_places;  i++) {
        pi += numerator / denominator;
        numerator = -numerator;
        denominator += 2;
    }
    pi *= 4;
    return pi;
}

These two improvements both have a measurable performance improvement when calculating a few thousand terms. A much smaller benefit comes from re-writing the loop condition:

while (num_places--) {
    ...
}

Complete program

Here's my final version. You can of course use your own main() if you still prefer the interactive loop.

long double compute_pi(unsigned long num_places)
{
    long double pi = 0.;
    long double numerator = 1;
    unsigned long denominator = 1;
    while (num_places--) {
        pi += numerator / denominator;
        numerator = -numerator;
        denominator += 2;
    }
    return pi * 4;
}

#include <iostream>
#include <iomanip>
#include <limits>
#include <string>
int main(int, char **argv)
{
    std::cout << std::setprecision(1 + std::numeric_limits<long double>::digits10);
    while (*++argv) {
        auto n = std::stoul(*argv);
        std::cout << compute_pi(n) << " (to " << n << " terms)" << std::endl;
    }
}

Program output

g++ -std=c++17 -fPIC -g -Wall -pedantic -Wextra -Wwrite-strings -Wno-parentheses -Weffc++ -O3 64297.cpp -o 64297
./64297 0 1 10 100 1000 10000 100000 1000000 10000000 100000000 1000000000
0 (to 0 terms)
4 (to 1 terms)
3.041839618929402211 (to 10 terms)
3.131592903558552764 (to 100 terms)
3.140592653839792927 (to 1000 terms)
3.141492653590043241 (to 10000 terms)
3.141582653589793492 (to 100000 terms)
3.141591653589793185 (to 1000000 terms)
3.141592553589793097 (to 10000000 terms)
3.141592643589794078 (to 100000000 terms)
3.141592652589795228 (to 1000000000 terms)

You can see that this algorithm converges very slowly. But now you have the structure, you can easily play with alternatives.

\$\endgroup\$

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.