HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMajor

Standardly deviated Fibonacci

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
deviatedstandardlyfibonacci

Problem

I'm new to C++ (about 4 weeks with no prior programming experience in other languages) and am writing a program that's supposed to do the following:

  • Define an array of 100 sequential numbers of type float.



  • Define an array of 20 Fibonacci numbers of type int.



  • Use an overloaded function to compute the mean of the array containing 100 numbers.



  • Use an overloaded function to compute the standard deviation of an array containing 100 numbers.



  • Use an overloaded function to compute the mean of the array containing 20 Fibonacci numbers.



  • Use an overloaded function to compute the standard deviation of the array containing 20 Fibonacci numbers.



```
#include "stdafx.h"
#include
#include
using namespace std;

const int SIZE_OF_GENERIC_ARRAY = 100;
const int SIZE_OF_FIBONACCI_ARRAY = 20;

double arithmeticAverage;
char sequenceType;

float Array[SIZE_OF_GENERIC_ARRAY];
int Fibonacci[SIZE_OF_FIBONACCI_ARRAY];

void fillGenericArray(int SIZE_OF_GENERIC_ARRAY);
void fillFibonacciArray(int SIZE_OF_FIBONACCI_ARRAY);

double mean(float[], int);
double mean(int[], int);

double standardDeviation(float[], int);
double standardDeviation(int[], int);

void outputMean();
void outputStandardDeviation();

int _tmain(int argc, _TCHAR* argv[])
{
cout > sequenceType;

if (sequenceType == 'G' || sequenceType == 'g')
{
fillGenericArray(SIZE_OF_GENERIC_ARRAY);
outputMean();
outputStandardDeviation();
}

else if (sequenceType == 'F' || sequenceType == 'f')
{
fillFibonacciArray(SIZE_OF_FIBONACCI_ARRAY);
outputMean();
outputStandardDeviation();
}

else
cout << "\n"
<< "Invalid input. Please type 'F' or 'G'. Thank you.";

return 0;
}

void fillGenericArray(int SIZE_OF_GENERIC_ARRAY)
{
int i = 0;

Array[0] = { 1 };

for (int i = 0; i < SIZE_OF_GENERIC_ARRAY; i++)
Array[i] = i + 1;

return;
}

void fillFibonacciArray(int SIZE_OF_FIBONACCI_ARRAY)
{
int i;

Array[0] = { 0 };

Solution

I have found a couple of things that could help you improve your code.
Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.
Avoid the use of global variables

It may make some sense to have SIZE_OF_GENERIC_ARRAY as a global variable, but not arithmeticAverage. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.
Pass needed variables explicitly

This is basically the other way of stating the previous item. As an example, you have a function fillGenericArrray() which takes an integer argument which is the size of the array. That's good, but it should also take the array to be filled as another argument. Putting that into practice, we get this:

void fillGenericArray(float array[], int arraysize)
{
   for (int i = 0; i < arraysize; ++i)
       array[i] = i + 1;
}


Note too that the initialization of the first element is already done within the loop and need not be done explicitly.
Use a function template for nearly identical functions

Your two versions of mean are identical except for the fact that one takes an array of float and the other an array of int. You could simplify that by using a template:

template
double mean(T array[], int arraysize)
{
   double sumOfElements = 0;
   for (int i = 0; i < arraysize; ++i)
       sumOfElements += array[i];
   return sumOfElements / arraysize;
}


Note also that I have also made a few other changes.

  • moved the declaration of i within the loop



  • changed the names of the passed variables



  • eliminated the arithmeticMean local variable



  • removed the spurious parentheses from the return statement



  • change from postincrement to preincrement for the loop variable



Precalculate loop invariants

A loop invariant is a value used within a loop construct that doesn't change. Right now your code recalculates the mean for every iteration of the for loop in the standardDeviation function. Since it doesn't change, it's better to calculate it just once and then use it repeatedly within the loop. Combining that with the ideas in the previous point, those two functions become this single templated function:

template
double standardDeviation(T array[], int arraysize)
{
   double tempSum = 0;
   double mymean = mean(array, arraysize);
   for (int i = 0; i < arraysize; ++i)
   {
       tempSum += pow((array[i] - mymean), 2);
   }
   return sqrt(tempSum / arraysize);
}


Eliminate unused variables

This code declares a variable i at the top of both fillGenericArray and fillFibonacciArray but then redefines it within the scope of the for loop in each. You can (and should) eliminate the redundant declaration of i at the top of each of those functions. Also, note that your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.
Have functions do just one thing

Your function outputMean() does two things: it calculates and outputs the mean. Better would be to do something like this:

std::ostream& outputMean(std::ostream &out, double mean)
    {
        return out << "\nThe mean is: " << mean << '\n';
    }


Or, because it's now quite simple, eliminate the function entirely and call it from within main() like this:

std::cout << "\nThe mean is: " << mean(Array, SIZE_OF_GENERIC_ARRAY) << '\n';


Note, too, that this eliminates an error in your existing code -- regardless of which selection is made, outputMean() and outputStandardDeviation() both always calculate the mean of Array and never that of Fibonacci.
Don't use std::endl when \n will do

Emitting std::endl actually flushes the stream as well as emitting a newline character. This takes extra time and code which you don't really need in your code. Instead just use \n in all cases in this particular code.
Update:

I have noticed that the code uses the same names for global variables, local variables and function parameters. This makes things confusing and would be best avoided. For example, your code has a global variable:

const int SIZE_OF_FIBONACCI_ARRAY = 20;


This is then used within main as:

fillFibonacciArray(SIZE_OF_FIBONACCI_ARRAY);


The code for that function is:

void fillFibonacciArray(int SIZE_OF_FIBONACCI_ARRAY)
{
   int i;

   Array[0] = { 0 };
   Array[1] = { 1 };

   for (int i = 2; i < SIZE_OF_FIBONACCI_ARRAY; i++)
       Array[i] = Array[i - 1] + Array[i - 2];

   return;
}


The problem is that there are two different things called SIZE_OF_FIBONACCI_ARRAY here, and I think the difference may be confusing you. Let's rewrite the the function a bit to make it easier to talk about:

```
void fillFibonacciArray(int mysize)
{
int i;

Array[0] = 0;
Array[1] = 1; // note that we don't need braces here

for (int i = 2; i < mysize; i++)
Array[i] = Array[i - 1]

Code Snippets

void fillGenericArray(float array[], int arraysize)
{
   for (int i = 0; i < arraysize; ++i)
       array[i] = i + 1;
}
template<typename T>
double mean(T array[], int arraysize)
{
   double sumOfElements = 0;
   for (int i = 0; i < arraysize; ++i)
       sumOfElements += array[i];
   return sumOfElements / arraysize;
}
template<typename T>
double standardDeviation(T array[], int arraysize)
{
   double tempSum = 0;
   double mymean = mean(array, arraysize);
   for (int i = 0; i < arraysize; ++i)
   {
       tempSum += pow((array[i] - mymean), 2);
   }
   return sqrt(tempSum / arraysize);
}
std::ostream& outputMean(std::ostream &out, double mean)
    {
        return out << "\nThe mean is: " << mean << '\n';
    }
std::cout << "\nThe mean is: " << mean(Array, SIZE_OF_GENERIC_ARRAY) << '\n';

Context

StackExchange Code Review Q#55883, answer score: 22

Revisions (0)

No revisions yet.