6
\$\begingroup\$

This program takes 10 Math SAT scores and 10 Verbal Sat scores from the user, computes the standard deviation and averages, then displays them in a tabular format. After, it displays them to the user and is sent to a text file. I haven't done input mismatch handling in quite a while so I used the methods that you see down below.

#include <cstdlib>
#include <iostream>
#include <iomanip> 
#include <string>
#include <fstream>
#include <cmath>
#include <cctype>

using namespace std;

/**
 * 
 * @return 0
 */
int main() 
{
    int sat[10][2], mathAvg, verbAvg;
    double mathStd, verbStd;

    void describe_program();
    void read_scores(int sat[10][2]);
    void compute_means(int sat[10][2], int& mathAvg, int& verbAvg);
    void compute_std(int sat[10][2],int mathAvg, int verbAvg, double& mathStd, double& verbStd) ;
    void show_results(int sat[10][2], int mathAvg, int verbAvg, double mathStd,  double verbstd);
    bool again();

    describe_program();

    do
    {
    read_scores(sat);
    compute_means(sat, mathAvg, verbAvg);
    compute_std(sat, mathAvg, verbAvg,  mathStd, verbStd);
    show_results(sat, mathAvg, verbAvg, mathStd, verbStd);
    }while(again());
    return 0;
}

/**
 * 
 */
void describe_program()
{
    cout<<"This program reads SAT scores from 10 students (Math and verbal scores)"
        <<" and allocates the scores as elements inside a two dimensional array."
        <<endl<<"The program will then calculate the mean Math SAT score, the mean "
        <<"Verbal SAT score, and the standard deviation for both subjects."<<endl<<
        "The results will be sent to the user in a tabular format and will then "
        <<"be sent to text file."<<endl; 
}

/**
 * 
 * @param sat
 */
void read_scores(int sat[10][2])
{
    for(int i = 0; i<10;i++)
    {
        for(int j = 0; j<2; j++)
        {
            if(j == 0){
                cout<<"Enter Math score below:"<<endl;
            }

            else{
                cout<<"Enter Verbal Score below:"<<endl;
            }    


            cin>>sat[i][j];  
        }
    }
}

/**
 * 
 * @param sat
 * @param mathAvg
 * @param verbAvg
 */
void compute_means(int sat[10][2], int& mathAvg, int& verbAvg)
{
    int mathScoreSum =0;
    int mathCount = 0;
    int verbScoreSum = 0;
    int verbCount = 0;

    for(int i = 0; i<10; i++)    
    {
        for(int j = 0; j<2; j++)
        {
            if(j==0){
                mathScoreSum += sat[i][j];
                mathCount++;
            }

            else{
                verbScoreSum += sat[i][j];
                verbCount++;
            }
        }
    }
    mathAvg = mathScoreSum / mathCount;
    verbAvg = verbScoreSum / verbCount;
}

/**
 * 
 * @param sat
 * @param mathAvg
 * @param verbAvg
 * @param mathStd
 * @param verbStd
 */
void compute_std(int sat[10][2],int mathAvg, int verbAvg, double& mathStd, double& verbStd) 
{
    double mathVariance = 0;
    double verbVariance = 0;

    for(int i = 0; i<10; i++)    
    {
        for(int j = 0; j<2; j++)
        {
            if(j==0){
                mathVariance += ::pow((sat[i][j] - mathAvg), 2);
            }

            else{
                verbVariance += ::pow((sat[i][j] - mathAvg), 2);
            }

            mathStd = sqrt(mathVariance/9);
            verbStd = sqrt(verbVariance/9);
        }
    }
}

/**
 * 
 * @param sat
 * @param mathAvg
 * @param verbAvg
 * @param mathStd
 * @param verbStd
 */
void show_results(int sat[10][2], int mathAvg, int verbAvg, double mathStd, double verbStd)
{ 
    cout<<fixed<<showpoint<<setprecision(1);
    cout<<setw(13)<<"MATH"<<setw(13)<<"VERBAL"<<endl;
    for(int i = 0; i<10;i++)
    {
        for(int j = 0; j<2;j++)
        {
            if(j==0){
                cout<<setw(13)<<sat[i][j];
            }else{
                cout<<setw(13)<<sat[i][j]<<endl;
            }
        }
    }
    cout<<"MEAN"<<setw(9)<<mathAvg<<setw(13)<<verbAvg<<endl;
    cout<<"STD"<<setw(10)<<mathStd<<setw(13)<<verbStd;

    ofstream outs;
    outs.open("scores.txt",ios::app);

    outs<<fixed<<showpoint<<setprecision(1);
    outs<<setw(13)<<"MATH"<<setw(13)<<"VERBAL"<<endl;
    for(int i = 0; i<10;i++)
    {
        for(int j = 0; j<2;j++)
        {
            if(j==0){
                outs<<setw(13)<<sat[i][j];
            }else{
                outs<<setw(13)<<sat[i][j]<<endl;
            }
        }
    }
    outs<<"MEAN"<<setw(9)<<mathAvg<<setw(13)<<verbAvg<<endl;
    outs<<"STD"<<setw(10)<<mathStd<<setw(13)<<verbStd<<endl;
    outs.close(); 
}

/**
 * 
 * @return Bool variable
 */
bool again()
{
    char response;

    cout<<endl<<"Do you wish to run this program again (Y or N)?"<<endl;
    cin>>response;     

    while((response!= 'N') && (response!= 'Y') && (response!= 'y') &&
          (response!= 'n')){
        cout<<"Please enter a valid response";
        cin>>response;
    }

    response = toupper(response);

    if(response == 'Y')
        return true;
    else
        return false;    
}
\$\endgroup\$
  • \$\begingroup\$ One up for applying Gray code in response!='N' && response!='Y' && response!='y' && response!='n', one down for if (condition) return true; else return false; \$\endgroup\$ – greybeard Feb 22 '18 at 0:08
3
\$\begingroup\$
  • using namespace std; is a bad practice

  • Repetitions inside the for(int j = 0; j<2; j++) loops violate the DRY principle. You'd be in a better shape computing means and averages independently for each score (to add Phys SAT you'd need to edit each function). Consider refactoring along the lines of

    int compute_mean(int sat[10]);
    double compute_std(int sat[10], int mean);
    ....
    

    calling them for each subject.

  • mean should be double. Rounding mean to an integer seriously biases a deviation. You may round it when printing.

  • The

    if(response == 'Y')
        return true;
    else
        return false;    
    

    is a long way to say

    return response == 'Y';
    
\$\endgroup\$
  • \$\begingroup\$ I know, sorry, my teacher is very old school and hasn't changed the assignment in a few years, as for the whole using namespace, well like I said, he hasn't been in the industry for quite some time so he probably didn't even know that using namespace std is considered bad practice, either way, thanks. \$\endgroup\$ – Darnell Mcdonald Feb 22 '18 at 14:03
3
\$\begingroup\$

Here are a few things that I see:

Function Declarations:

void describe_program();
void read_scores(int sat[10][2]);
void compute_means(int sat[10][2], int& mathAvg, int& verbAvg);
void compute_std(int sat[10][2],int mathAvg, int verbAvg, double& mathStd, double& verbStd) ;
void show_results(int sat[10][2], int mathAvg, int verbAvg, double mathStd,  double verbstd);
bool again();

Why do you declare your functions inside main? Function protypes are usually kept outside main, since they interfere less with the logic of your code.


Raw Multidimensional Arrays

Using raw multidimensional arrays is bad, since they easily decay to pointers and don't know their own size. Consider implementing your own matrix class, or going with a nested std::array/std::vector.

The other thing I question is why you are even using a multidimensional array. The two series of data are not holding the same score, so I would opt for two different std::vectors for each set of scores. This would allow you to simplify your functions a bit, although you might have to call them twice.


Use of endl

I notice that you add endl whenever you want a newline. Beware though, this also flushes the output with it, which could have a toll on performance. Consider switching to \n.


::pow

Another reason why using namespace std; is considered bad practice.


Refactoring show_results()

Looking at your function show_results:

void show_results(int sat[10][2], int mathAvg, int verbAvg, double mathStd, double verbStd)
{ 
    cout<<fixed<<showpoint<<setprecision(1);
    cout<<setw(13)<<"MATH"<<setw(13)<<"VERBAL"<<endl;
    for(int i = 0; i<10;i++)
    {
        for(int j = 0; j<2;j++)
        {
            if(j==0){
                cout<<setw(13)<<sat[i][j];
            }else{
                cout<<setw(13)<<sat[i][j]<<endl;
            }
        }
    }
    cout<<"MEAN"<<setw(9)<<mathAvg<<setw(13)<<verbAvg<<endl;
    cout<<"STD"<<setw(10)<<mathStd<<setw(13)<<verbStd;

    // ... Basically the same thing for a file
}

It is immediately noticeable that you are repeating yourself for output to the console and output to the file. Instead, I would change the function so that it takes a std::ostream as a parameter and sends all output to this stream. Then, in main, you can call show_results() twice, once for standard output, and once for a handle to a file. Here is how I would implement this change:

void show_results(const ostream& out, int sat[10][2], int mathAvg, int verbAvg, double mathStd, double verbStd)
{ 
    out<<fixed<<showpoint<<setprecision(1);
    out<<setw(13)<<"MATH"<<setw(13)<<"VERBAL"<<endl;
    for(int i = 0; i<10;i++)
    {
        for(int j = 0; j<2;j++)
        {
            if(j==0){
                out<<setw(13)<<sat[i][j];
            }else{
                out<<setw(13)<<sat[i][j]<<endl; // Make sure to replace your endls
            }
        }
    }
    out<<"MEAN"<<setw(9)<<mathAvg<<setw(13)<<verbAvg<<endl;
    out<<"STD"<<setw(10)<<mathStd<<setw(13)<<verbStd;
}

Refactored Conditional

if(response == 'Y')
    return true;
else
    return false;

Can be replaced with:

return response == 'Y';
\$\endgroup\$
0
\$\begingroup\$

You can also reduce the lengthiness of computing averages using lambdas, like with the below vector:

std::vector<int> v = { 1, 2, 3, 4, 2314, 6, 7, 8, 9, 10 };
auto lambda = [&](double a, double b){return a + b / v.size(); };
double avg = std::accumulate(v.begin(), v.end(), 0.0, lambda);
\$\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.