Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written a little program to calculate pi using the Nilakantha series:

For this formula, take three and start alternating between adding and subtracting fractions with numerators of 4 and denominators that are the product of three consecutive integers which increase with every new iteration. Each subsequent fraction begins its set of integers with the highest one used in the previous fraction. Carry this out even a few times and the results get fairly close to pi. (http://www.wikihow.com/Calculate-Pi)

I don't really understand other ways of calculating pi, or they take too long to perform.

echo "Enter scale please"
read SCALE
VALUE=2
PI=3
FITNESS=1
while true
do
PI=$(echo "scale=$SCALE;$PI+(4/($VALUE*($VALUE+1)*($VALUE+2)))-(4/(($VALUE+2)*($VALUE+3)*($VALUE+4)))" | bc)
VALUE=$(($VALUE+4))
FITNESS=$(($FITNESS+1))
echo "###############"
echo "--> $FITNESS // $VALUE"
echo "$PI"
done

I would really like to know how to detect when I get the same output multiple times, so I can tell the program to terminate when it has reached the most accurate version of pi possible.

Also if you have other suggestions on how to improve the code and/or know a better way of calculating pi (with some explanation), I would like to hear.

share|improve this question

2 Answers 2

I see a few things that could allow you to improve your program. First, though, I don't consider myself a bash expert, so there may well be better ways of doing these things.

Use a "shebang" line

As this question points out, you should always use a "shebang" line for your bash scripts. So the first line would be:

#!/usr/bin/env bash

Pass values as arguments

Rather than prompting for the SCALE value, it's generally better to use a command line argument. That way, the script can be reused by other shell scripts.

Provide a stopping mechanism

As each term is calculated, eventually, it will be equal to zero given the passed scale. This suggests a mechanism for stopping: check each term for 0 before adding it.

Indent do and while loops

I don't know of a bash style guide (there probably is one!) but I like to see the contents of loops indented to make it easier to read.

Putting it all together

Here's a modification of your script with all of these suggestions implemented:

bashpi.sh

#!/usr/bin/env bash
SCALE=$1
VALUE=2
PI=0
FITNESS=1
DELTA=3
while [ $(echo "scale=$SCALE;$DELTA==0" |bc) != "1" ]
do
    PI=$(echo "scale=$SCALE;$PI+$DELTA" | bc)
    DELTA=$(echo "scale=$SCALE;(4/($VALUE*($VALUE+1)*($VALUE+2)))-(4/(($VALUE+2)*($VALUE+3)*($VALUE+4)))" | bc)
    VALUE=$(($VALUE+4))
    FITNESS=$(($FITNESS+1))
    echo "###############"
    echo "--> $FITNESS // $VALUE"
    echo "$PI"
done

To better understand how this works, you can replace the three echo statements with this one:

echo "DELTA = ${DELTA} --> ${FITNESS} // ${VALUE} : ${PI}"

Sample output

With ./bashpi.sh 4, and the modified echo above I get this output:

DELTA = .1333 --> 2 // 6 : 3
DELTA = .0064 --> 3 // 10 : 3.1333
DELTA = .0012 --> 4 // 14 : 3.1397
DELTA = .0003 --> 5 // 18 : 3.1409
DELTA = .0001 --> 6 // 22 : 3.1412
DELTA = .0001 --> 7 // 26 : 3.1413
DELTA = .0001 --> 8 // 30 : 3.1414
DELTA = 0 --> 9 // 34 : 3.1415
share|improve this answer
    
I really appreciate the suggestions, but I don't completely understand how you've implemented the stopping feature, SCALE simply is how many numbers exist after the decimal point. There's probably something I'm not seeing here... –  pi.sasha 17 hours ago
    
It's a bit subtle. Basically, a value of 0.001 (=1e-3) with a scale of 3 is nonzero, but a value of 0.0005 (=5e-4) is considered to be equal to zero at a scale of 3, so when the next term becomes less than smaller than 1e-${SCALE}, the printable part of ${PI} is unlikely to change except in the least significant digit. A refinement would be to use ${SCALE}+1 in the while loop but it doesn't make much difference and this is slightly easier to understand. –  Edward 15 hours ago

Edward's answer offers the necessary insights to almost all the parts of the original inquiry. I'd like to add my two cents on the "if you have other suggestions on how to improve the code and/or know a better way of calculating pi (with some explanation), I would like to hear" part.

The following script is a compilation of my findings of calculating pi in bash on similar sites:

#!/usr/bin/env bash

: ${SCALE:=${1:-5}}
: ${_PROF_INIT:=-1}
: ${_PROF_START:=0}

function _profiler_calc () { 
    echo "$@" | bc -l | xargs printf "%5.0f\n"
}

function _profiler_ts() {
    date +%s%3N
}

function _profiler_mark() {
    _PROF_START=$(_profiler_ts)
    if [ "x${_PROF_INIT}" == "x-1" ]; then
        _PROF_INIT=${_PROF_START}
    fi
}

function _profiler_elapsed() {
    NOW="$(_profiler_ts)"
    ELAPSED=$(_profiler_calc "${NOW} - ${_PROF_START}")
    printf "%s\n" "$ELAPSED"
}

# http://codereview.stackexchange.com/questions/94266/calculating-pi-using-bash
function calc_edward() {
    local -i SCALE=$1
    local -i VALUE=2
    local -i FITNESS=1
    local DELTA=3
    local PI=0
    if [ $SCALE -gt 10 ]; then
        echo "n/a"
        return
    fi
    while [ $(echo "scale=$SCALE;$DELTA==0" | bc) != "1" ]; do
        PI=$(echo "scale=$SCALE;$PI+$DELTA" | bc)
        DELTA=$(echo "scale=$SCALE;(4/($VALUE*($VALUE+1)*($VALUE+2)))-(4/(($VALUE+2)*($VALUE+3)*($VALUE+4)))" | bc)
        VALUE=$(($VALUE+4))
        FITNESS=$(($FITNESS+1))
    done
    echo $PI
}

# http://superuser.com/questions/275516/how-can-i-generate-pi-to-a-given-number-of-decimal-places-from-a-script
function calc_8088() {
    local -i SCALE=$1
    bc -l <<< "scale=$SCALE; 4*a(1)"
}

# http://stackoverflow.com/questions/23524661/how-can-i-calculate-pi-using-bash-command
function calc_devnull() {
    local -i SCALE=$1
    { echo -n "scale=$SCALE;"; seq 1 2 $((2*SCALE)) | xargs -n1 -I{} echo '(16*(1/5)^{}/{}-4*(1/239)^{}/{})';} | paste -sd-+ | bc -l
}

# http://unix.stackexchange.com/questions/166220/how-do-i-print-pi-3-14159/166582#166582
function calc_pabouk() {
    local -i SCALE=$1
    bc -l <<< "s=$((SCALE+1)); scale=s+2; pi=4*a(1)+5*10^(-s); scale=s-1; pi/1"
}

_profiler_mark
pi=$(calc_edward ${SCALE})
printf "%15s: %$((SCALE+2))s %-$((SCALE+1))s\n" "Edward's Pi" "$pi" "[time:$(_profiler_elapsed)ms]"

_profiler_mark
pi=$(calc_8088 ${SCALE})
printf "%15s: %s %-$((SCALE+1))s\n" "8088's Pi" "$pi" "[time:$(_profiler_elapsed)ms]"

_profiler_mark
pi=$(calc_devnull ${SCALE})
printf "%15s: %s %-$((SCALE+1))s\n" "devnull's Pi" "$pi" "[time:$(_profiler_elapsed)ms]"

_profiler_mark
pi=$(calc_pabouk ${SCALE})
printf "%15s: %s %-$((SCALE+1))s\n" "pabouk's Pi" "$pi" "[time:$(_profiler_elapsed)ms]"

I have added the pointers to the original code inside my script. If you run it, you'll get some interesting output:

$ for s in $(seq 2 4 30); do ./bashpi.sh $s; done
    Edward's Pi: 3.14 [time:   63ms]
      8088's Pi: 3.12 [time:   14ms]
   devnull's Pi: 3.20 [time:   21ms]
    pabouk's Pi: 3.14 [time:   14ms]
    Edward's Pi: 3.141591 [time:  336ms]
      8088's Pi: 3.141592 [time:   14ms]
   devnull's Pi: 3.141595 [time:   30ms]
    pabouk's Pi: 3.141593 [time:   15ms]
    Edward's Pi: 3.1415926488 [time: 2879ms]
      8088's Pi: 3.1415926532 [time:   14ms]
   devnull's Pi: 3.1415926538 [time:   41ms]
    pabouk's Pi: 3.1415926536 [time:   14ms]
    Edward's Pi:              n/a [time:   10ms]
      8088's Pi: 3.14159265358976 [time:   14ms]
   devnull's Pi: 3.14159265358981 [time:   48ms]
    pabouk's Pi: 3.14159265358979 [time:   14ms]
    Edward's Pi:                  n/a [time:   10ms]
      8088's Pi: 3.141592653589793236 [time:   15ms]
   devnull's Pi: 3.141592653589793243 [time:   59ms]
    pabouk's Pi: 3.141592653589793238 [time:   14ms]
    Edward's Pi:                      n/a [time:   11ms]
      8088's Pi: 3.1415926535897932384624 [time:   15ms]
   devnull's Pi: 3.1415926535897932384628 [time:   74ms]
    pabouk's Pi: 3.1415926535897932384626 [time:   14ms]
    Edward's Pi:                          n/a [time:   10ms]
      8088's Pi: 3.14159265358979323846264336 [time:   14ms]
   devnull's Pi: 3.14159265358979323846264340 [time:   88ms]
    pabouk's Pi: 3.14159265358979323846264338 [time:   15ms]
    Edward's Pi:                              n/a [time:   10ms]
      8088's Pi: 3.141592653589793238462643383276 [time:   14ms]
   devnull's Pi: 3.141592653589793238462643383281 [time:  111ms]
    pabouk's Pi: 3.141592653589793238462643383280 [time:   14ms]
share|improve this answer

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

2  
This answer does link back to the OP's question in a sense that the OP does ask for alternative suggestions. I've restored the edits to make that link clear. On the other hand, this answer is a poor review because it does not explain why it is better or what advantages it has. It is not a review. I encourage you to either elaborate on why this code is an improvement, or ask it as a separate question for its own code review. –  rolfl 9 hours ago

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.