7
\$\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\$
11
  • 4
    \$\begingroup\$ You can also use getchar()... \$\endgroup\$ Nov 22, 2013 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, 2013 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\$ Nov 23, 2013 at 0:46
  • \$\begingroup\$ I realized, this was because I added an extra symbol last minute. \$\endgroup\$ Nov 23, 2013 at 0:48
  • \$\begingroup\$ @Jamal yeah right... I overlooked the C++ tag! \$\endgroup\$ Nov 23, 2013 at 8:13

3 Answers 3

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\$
4
  • \$\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, 2013 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, 2013 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, 2013 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, 2013 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\$
4
  • 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, 2013 at 21:27
  • 1
    \$\begingroup\$ And then there are more good options like giving that struct an operator>> or similar helper method... \$\endgroup\$ Nov 23, 2013 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, 2013 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, 2013 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\$
5
  • 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, 2013 at 21:22
  • \$\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, 2013 at 21:31
  • \$\begingroup\$ @Jamal Oh, good point, I only forgot to remove this assignment I casually added. ;-) \$\endgroup\$
    – Wolf
    Nov 24, 2013 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, 2013 at 17:25
  • \$\begingroup\$ @jliv902 good idea, indeed. What about adding your own appendix? I'll better not to incorporate this suggestion, because it cannot handle the problem with the shared top rank. \$\endgroup\$
    – Wolf
    Nov 25, 2013 at 21:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.