6
\$\begingroup\$

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

enter image description here

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;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ You can also use getchar()... \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 22 '13 at 19:27
  • \$\begingroup\$ @tintinmj: std::cin.get() would be preferred for C++. It also goes with <iostream>, whereas getchar() goes with <cstdio>/<stdio.h>. \$\endgroup\$ – Jamal Nov 22 '13 at 23:19
  • \$\begingroup\$ Your "% of Total Hours" column doesn't line up with its header. Should be a simple enough thing to do properly, and it just looks sloppy if you don't. \$\endgroup\$ – Darrel Hoffman Nov 23 '13 at 0:46
  • \$\begingroup\$ I realized, this was because I added an extra symbol last minute. \$\endgroup\$ – Quaxton Hale Nov 23 '13 at 0:48
  • \$\begingroup\$ @Jamal yeah right... I overlooked the C++ tag! \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 23 '13 at 8:13
10
\$\begingroup\$
  • 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;
    }
    
\$\endgroup\$
  • \$\begingroup\$ @Jamal I would consider your percent[0] dereferencing before the loop a little dangerous. Even if the homework states that at least 2 students form a team, I would not trust in this. \$\endgroup\$ – Wolf Nov 24 '13 at 21:23
  • \$\begingroup\$ @Wolf: This is what the OP has in the code, minus the numerical types. That apecific part I didn't look at closely. I suppose this means that the entire program should use vectors instead of C-style arrays. \$\endgroup\$ – Jamal Nov 24 '13 at 21:28
  • \$\begingroup\$ Yes, nobody can learn all at one time. And the OP obviously didn't ask for a vector tutorial. \$\endgroup\$ – Wolf Nov 24 '13 at 21:38
  • \$\begingroup\$ @Wolf: Right, but it could still help to mention it, even if it cannot be used in this assignment. Heck, I'm not opposed to going back to old assignments and improving on them. Many can make great references! \$\endgroup\$ – Jamal Nov 24 '13 at 21:45
10
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ D'oh! I forgot all about structs. I was merely going by the OP's solution, but this could also work. +1 from me. \$\endgroup\$ – Jamal Nov 22 '13 at 21:27
  • 1
    \$\begingroup\$ And then there are more good options like giving that struct an operator>> or similar helper method... \$\endgroup\$ – Michael Urman Nov 23 '13 at 2:36
  • \$\begingroup\$ Great answer (from the code review POV). I think, you agree in this, that the percentage shouldn't be stored along the real input, which is name and hours? \$\endgroup\$ – Wolf Nov 24 '13 at 20:23
  • \$\begingroup\$ @Wolf, Yeah I agree. Percentage information would be redundant. A function should be used to calculate that information. \$\endgroup\$ – jliv902 Nov 25 '13 at 17:10
2
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Highest()'s parameter should also be const (and the iterators could also be .cbegin() and .cend()). It would also be more readable to use i->hours instead of (*i).hours). \$\endgroup\$ – Jamal Nov 24 '13 at 21:22
  • \$\begingroup\$ @Jamal Thanks for the hint, I'll update this. \$\endgroup\$ – Wolf Nov 24 '13 at 21:24
  • \$\begingroup\$ I've also never considered initializing most with "nobody", which seems pretty obvious now. Can't possibly identify every little point, though. ;-) \$\endgroup\$ – Jamal Nov 24 '13 at 21:31
  • \$\begingroup\$ @Jamal Oh, good point, I only forgot to remove this assignment I casually added. ;-) \$\endgroup\$ – Wolf Nov 24 '13 at 21:34
  • 1
    \$\begingroup\$ For Highest(), using std::max_element from algorithm.h would be better than rolling your own loop. \$\endgroup\$ – jliv902 Nov 25 '13 at 17:25

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.