2
\$\begingroup\$

You are a computer science professor at South Harmon Institute of Technology, and are in dire need of automatic grading! The good news is you have all of your student's assignments in an easy-to-read format, making automation easy!

You will be given a list of unique student names, and then a list of their assignment grades. All assignments are based on 20 points and are scored in whole-numbers (integers). All students have received the same number of assignments, so you don't have to worry about managing jagged arrays.


#include <iostream>
#include <string>
#include <vector>

class grades {
public:
    friend std::ostream &output(std::ostream &os, grades&);
        friend bool inputGrades(grades &test, const std::vector<std::string> &students, const std::vector<double> &tests);

    bool setAmmount(unsigned, unsigned);
    void genGrades();
    void clear() {numStudents = 0; numTests = 0;}   
private:

    std::vector<double> getGrades() const {return Grades;} 
    std::vector<std::string> getStudents() const {return Names;}

    unsigned numStudents;
    double numTests;
    double totalNumTests;
    std::vector<double> Grades;
    std::vector<std::string> Names;
};

    bool grades::setAmmount(unsigned nStud, unsigned nTest) {
    if(nStud >= 1 && nStud <= 60 && nTest >= 4 && nTest <= 100) {
        numStudents = nStud;
        numTests = nTest;
        totalNumTests = nStud * nTest;
        return true;
    }
    return false;
}

bool inputGrades(grades &test, const std::vector<std::string> &students, const std::vector<double> &tests) {
    if(test.totalNumTests != tests.size() || test.numStudents != students.size())
        return false;
    test.Grades = tests;
    test.Names = students;
    return true;
}

void grades::genGrades() {
    unsigned tempNum = 0; double TempGrade = 0; std::vector<double> newGrades;
    if(Grades.size() != totalNumTests || Names.size() != numStudents) { 
        return;
    }
    for(auto c = Grades.begin(); c != Grades.end(); ++c) {
    ++tempNum;
    TempGrade += *c;
        if (tempNum == numTests) {
            TempGrade /= tempNum;
            newGrades.push_back(TempGrade);
            TempGrade = 0; tempNum = 0;
        }
    }
    Grades = newGrades;
    return;
}

std::ostream &output(std::ostream &os, grades &test) {
    auto grades = test.getGrades(); auto students = test.getStudents();
    auto size = students.size();
    for(unsigned cnt = 0; cnt != size; ++cnt) {
        os << students[cnt] << " " << grades[cnt] << std::endl;
    }
    return os;
}

Expected input/output:

Input:

3 5

JON 19 14 15 15 16

JEREMY 15 11 10 15 16

JESSE 19 17 20 19 18

Output:

15.93

JON 15.80

JEREMY 13.40

JESSE 18.60

\$\endgroup\$
  • 1
    \$\begingroup\$ I notice that you changed the code in the question 2 minutes before the first answer. I doubt the answering person reviewed that revision. In general, avoid changing code after you've posted a question. \$\endgroup\$ – Sumurai8 Jan 17 '16 at 20:04
  • 1
    \$\begingroup\$ I noticed just noone answered yet so I changed it up but the changes I made didnt change the answer that much \$\endgroup\$ – Magirldooger Jan 17 '16 at 20:13
4
\$\begingroup\$

What is grades actually?

What is this class supposed to represent? It has a vector<int> for the individual test grades, which leads me to believe that based on your example, we might have one grades for JON containing {19, 14, 15, 15, 16} and another for JEREMY, etc. But then you also have a vector<string> for the students. So it's unclear what the usage of this class is. It's trying to do too many things - that's bad design.

This is made all the more confusing by genGrades() which alters the contents of Grades to be something else entirely. You also have issues with integer division - you have no way of being able to print 15.80 since nothing is ever a double or float.

A better design would be to separate your concerns. Have a class that is an individual student. A student has a name and some scores (don't keep reusing the word "grades" to mean different things):

struct Student {
    std::string name;
    std::vector<int> scores;
};

A student might also know how to compute their own average:

struct Student {
    std::string name;
    std::vector<int> scores;

    double average() const;
};

And then all you need is a container of them:

std::vector<Student> students;

This makes it clear what's going on. The average() member function will give you the average score, without any modification. That will make it easy to reason about and have the code much clearer. For instance, most of the output is just:

for (const Student& student : students) {
    std::cout << student.name << " " << student.average() << '\n';
}

Class I/O in C++

The way you make classes streamable in C++ is with operator<<, not a function named output(). The reason for this is so you can write:

grades g(...);
std::cout << "Grades are: " << g;

Similarly, input should be spelled operator>> so that you could write:

grades g;
std::cin >> g;

It makes the interaction the same with your class as it is with any other class.

That said, your inputGrades() doesn't actually input anything. It seems more like a constructor:

grades(const std::vector<int>&, const std::vector<std::string>& );

One line per line

One of the things that makes it very hard to read your code is you do things like:

auto grades = test.getGrades(); auto students = test.getStudents();

There is no reason for that! Vertical space isn't at a premium, and it is MUCH easier to read this:

auto grades = test.getGrades();
auto students = test.getStudents();

(Although since both of those functions return by-value, you're making unnecessary copies here as well. Prefer to return by reference to const, and then don't alias them. )

\$\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.