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

Hour averaging program

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

Problem

I am looking for tips on improving my short program. I am using system("PAUSE") because this was for an assignment.

Code:

#include 
#include 
#include 
using namespace std;

void Display(string* , int* , double*, int);
void Percentage(int*, double*, int);
void Intro(string*, int*, int&);
void Highest(int&, string&, double*, string*, int);

int main()
{
    int students;
    string names[10];
    int hours[10];
    double percents[10];
    int highest;
    string most;

    Intro(names, hours, students);

    Percentage(hours, percents, students);
    Display(names, hours, percents, students);

    Highest(highest, most, percents, names, students);

    system("PAUSE");
}

void Intro(string* names, int* hours, int& students)
{
    int team;
    cout > team;

    cout > names[i] >> hours[i];
    }

    students = team;
}

void Display(string* names, int* hours, double* percent, int students)
{
    cout << setw(20) << "Students";
    cout << setw(20) << "Hours Worked";
    cout << setw(20) << "% of Total Hours";
    cout << endl;
    cout << "------------------------------------------------------------" << endl;
    for (int i = 0; i < students; i++)
    {
        cout << setw(20) << names[i];
        cout << setw(9) << hours[i];
        cout << setw(16) << percent[i];
        cout << endl;
    }
}

void Percentage(int* hours, double* percent, int students)
{
    int total(0);

    for (int i = 0; i < students; i++)
    {
        total += hours[i];
    }

    for (int i = 0; i < students; i++)
    {
        percent[i] = double(hours[i]) / total * 100;
    }
}

void Highest(int& highest, string& most, double* percent, string* names, int students)
{
    highest = percent[0];

    for (int i = 0; i < students; i++)
    {
        if (highest < percent[i])
        {
            highest = percent[i];
            most = names[i];
        }
    }

    cout << most << " worked the most hours." << endl;
}

Solution

-
I assume you'll have no more than 10 names/hours/percents, but you could use an std::vector in place of these arrays. This will allow you any number of inputs without fear of exceeding the allotted 10. If you don't need this for your assignment, then you can stick with what you have.

-
If you're allowed to use more of the STL, I'd recommend std::accumulate for summing up the values in a container.

For instance, your first loop in Percentage():

for (int i = 0; i < students; i++)
{
    total += hours[i];
}


can be done with this function as such (with the array):

// the 10 corresponds to the array size
// the 0 is the starting value of the accumulator

int total = std::accumulate(hours, hours+10, 0);


If you choose to use std::vector (or other STL container):

// functions cbegin() and cend() return const iterators

int total = std::accumulate(hours.cbegin(), hours.cend(), 0);


-
I'd slightly tweak the percentage calculation for clarity:

percent[i] = (double(hours[i]) / total) * 100;


-
Prefer to cast the C++ way:

// C way
double(hours[i])


// C++ way
static_cast(hours[i])


-
It seems a little weird having Highest() display something. You could instead return most, thereby making the function of type std::string. That way, you can display this name from main() or wherever else.

You don't need the first two arguments. They are passed in from main(), but also not modified by any previous functions. I'd remove those two and just use local variables.

highest should be of type double since it's being assigned to a percent element. Otherwise, you may receive a "possible loss of data" warning. This is especially important for when the two values are being compared in the loop.

You could then have this:

// get the name
std::string most = Highest(percents, names, students);

// display it
std::cout << most << " worked the most hours." << std::endl;


std::string Highest(double* percent, string* names, int students)
{
    double highest = percent[0];
    std::string most;

    for (int i = 0; i < students; i++)
    {
        if (highest < percent[i])
        {
            highest = percent[i];
            most = names[i];
        }
    }

    return most;
}

Code Snippets

for (int i = 0; i < students; i++)
{
    total += hours[i];
}
// the 10 corresponds to the array size
// the 0 is the starting value of the accumulator

int total = std::accumulate(hours, hours+10, 0);
// functions cbegin() and cend() return const iterators

int total = std::accumulate(hours.cbegin(), hours.cend(), 0);
percent[i] = (double(hours[i]) / total) * 100;
// C way
double(hours[i])

Context

StackExchange Code Review Q#35941, answer score: 10

Revisions (0)

No revisions yet.