Hour averaging program

Question

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

Code:

#include <iostream>
#include <string>
#include <iomanip>
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 << "A team is made up of atleast 2 students. How many students are on the team?: ";
    cin >> team;

    cout << "Enter student's first name and the hours worked on the final project: " << endl;

    for (int i = 0; i < team; i++)
    {
        cout << i + 1 << ": ";
        cin >> 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;
}

Answer 1

• 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<double>(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;
  }
  

Answer 2

To add to what Jamal is saying. If you are allowed to used basic structs, you could do something like this:

struct Student
{
    std::string name ;
    int hours ;
};

static void Intro (std::vector <Student> &students) ;
static void Display (const std::vector <Student> &students) ;
// ...  

This way, you don't have to manage parallel arrays.

Answer 3

Again, in addition to jliv902's addition - and since we are here for code review - I think the Highest function needs only names and hours (and therefore std::vector<Student> which I shorten as Students via typedef):

typedef std::vector<Student> Students;

std::string Highest(const Students& students)
{
    int highest = 0;
    std::string most = "nobody";

    for (Students::const_iterator i=students.begin(); i!=students.end(); ++i) {
        if (highest < (*i).hours) {
            highest = (*i).hours;
            most = (*i).name;
        }
    }
    return most;
}

So also percents are for output only and need not to be stored along with the real input. As a pragmatic introduction to std::vector, the according Intro function will do best:

void Intro(Students& students)
{
    int teamSize = 0;
    cout << "A team is made up of atleast 2 students. How many students are on the team?: ";
    cin >> teamSize;

    cout << "Enter student's first name and the hours worked on the final project: " << endl;

    for (int i = 0; i < teamSize; i++) {
        cout << i + 1 << ": ";
        Student s;
        cin >> s.name >> s.hours;
        students.push_back(s);
    }
}

BTW: Would you be satisfied with a solution, where two or more students share the first place in hours but only one of them is put out as the top worker?

EDIT: this simple approach allows for a list of top workers instead of single one:

typedef std::list<std::string> Names;

/// output the name(s) of the student(s) that have worked the most hours
Names HighestList(const Students& students)
{
    int highest = 0;
    Names result;

    for (Students::const_iterator i=students.begin(); i!=students.end(); ++i) {
        if (highest < i->hours) {
            highest = i->hours;
            result.clear();
            result.push_back(i->name);
        } else if (highest == i->hours) {
            result.push_back(i->name);
        }
    }
    return result;
}