Read in file, and then compute class mean, median, max, min, class averages & update file

Question

The application is designed to read in class results of individual students from a single text file, and then manipulate the data to give information such as class mean, median, max, min, individual student averages and updating the result file if need be.

The format of the text file for each student is as such:

2            // States how many Test scores are attributed to student ID
S1234567     // Student  ID
55           // Test score 1
70           // Test score 2

Software Code:

*/
Variables Used:
------------------
int limit                   // States the amount of test data per student 
int readBuffer              // Skips the first two array values before the mark data
int skipMarkData            // Skips the marks data so that i is ready to start at the start of the next student
double sum                  // Totalling the sum of the student individual average
fileWriter                  // Used to write changes to file
fileReader                  // Used to read from file
string currentReadLine      // Reads the current line within the file
vector<int> numberStore     // Temp storage for string test data that is converted to integer for math processing
vector<string> marksArray   // Stores the contents of the file --> marks.txt
char choice                 // Stores users choice in regards to whether they want to continue data entry or not
string markInput            // Stores the user test data inputs as string for 
int amountOfTestScores      // Records the amount of test data the user has currently inputted
vector<string>addMarks      // Stores user data entry input
string intToStrOfTestScores // Converts the amountOfTestScores integer value into a string value for inclusion into addMarks array vector.
string studentID            // Stores the studentID value for lookup & data entry
int userInput               // Stores the menu selection values 1-8

*/
#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>
#include <vector>
#include <algorithm>
#include <numeric> 

//using namespace std;

bool readFile(const std::string fileName, std::vector<std::string>&marksArray) {
    std::string currentReadLine;
    std::ifstream fileReader;

    //Opens the file
    fileReader.open(fileName.c_str());

    if (fileReader.fail()) {
        std::cout << "\nThe file failed to open!"
                  << "\nPlease check that the file exists. Press any key to continue"
                  << std::endl;
        std::cin.ignore(256, '\n');             //Stops the program from skipping ahead
        std::cin.get();                     //Pauses program
        return false;
    }
    else {
        //Reads the file and stores into array line by line
        while (getline(fileReader, currentReadLine)) {
            marksArray.push_back(currentReadLine);
        }
        fileReader.close();

        return true;
    }
}

void displayMark(const int arrayItemCount, std::vector<std::string>marksArray) {
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        std::cout << "\n================================="
                  << "\nStudent ID: " << marksArray[i + 1];

        // Loops through & outputs student test score
        for (int z = 0; z < limit; z++) {
            std::cout << "\nTest " << z << ": " << marksArray[readBuffer + z];
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }
}

double findAverageForStudent(const int arrayItemCount, std::vector<std::string>marksArray, const std::string studentID) {
    double sum = 0;

    //Goes through the string vector and places the test scores into a int vector
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string value within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        if (studentID == marksArray[i + 1]) {
            // Loops through test score & stores them in a int vector array.
            for (int z = 0; z < limit; z++) {
                sum += atoi(marksArray[readBuffer + z].c_str());
            }
            //Calculate & return average for the student scores
            return (sum / limit);
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }

    std::cout << "\nRECORD NOT FOUND! Press any key to continue";
    std::cin.ignore(256, '\n');             //Stops the program from skipping ahead
    std::cin.get();                     //Pauses program
    return -1;
}




bool updateFile(const std::string fileName, std::vector<std::string>addMarks) {
    std::string currentReadLine;
    std::fstream fileWriter;

    //Opens the file
    fileWriter.open(fileName.c_str());

    if (fileWriter.fail()) {
        std::cout << "\nThe file failed to open!"
            << "\nPlease check that the file exists.Press any key to continue."
            << std::endl;
        std::cin.ignore(256, '\n');             //Stops the program from skipping ahead
        std::cin.get();                     //Pauses program
        return false;
    }
    else {
        fileWriter.seekg(0L, std::ios::end);        //Move to the end of the file

                                            //Loops through vector and writes the elements into file
        for (int i = 0; i < (addMarks.size()); i++) {
            fileWriter << "\n" << addMarks[i];
        }
        fileWriter.close();
        std::cout << "\nSuccessfully written changes! Press any key to continue";
        std::cin.ignore(256, '\n');             //Stops the program from skipping ahead
        std::cin.get();                     //Pauses program
        return true;
    }
}

double calculateMean(const int arrayItemCount, std::vector<std::string>marksArray) {
    std::vector<int> numberStore;                           // Temp storage of numerical test data

    //Goes through the string vector and places the test scores into a int vector
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        // Loops through test score & stores them in a int vector array.
        for (int z = 0; z < limit; z++) {
            numberStore.push_back(atoi(marksArray[readBuffer + z].c_str()));
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }
    // Return the mean by adding up all the values from begin to end then dividing
    return std::accumulate(numberStore.begin(), numberStore.end(), 0.0) / numberStore.size(); ;
}

double calculateMedian(const int arrayItemCount, std::vector<std::string>marksArray) {
    std::vector<int> numberStore;                           // Temp storage of numerical test data

    //Goes through the string vector and places the test scores into a int vector
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        // Loops through test score & stores them in a int vector array.
        for (int z = 0; z < limit; z++) {
            numberStore.push_back(atoi(marksArray[readBuffer + z].c_str()));
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }

    //Sort the vector
    std::sort(numberStore.begin(), numberStore.end());

    //Return the median //
    if (int div = numberStore.size() / 2 == 1) {
        return numberStore[numberStore.size() / 2];                                                             //Returns the median if vector size odd
    }
    return ((numberStore[(numberStore.size() / 2) - 1] + numberStore[(numberStore.size() / 2) + 1]) / 2);       //Returns the median if vector size even
}

double findMinimum(const int arrayItemCount, std::vector<std::string>marksArray) {
    std::vector<int> numberStore;                           // Temp storage of numerical test data

    //Goes through the string vector and places the test scores into a int vector
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        // Loops through test score & stores them in a int vector array.
        for (int z = 0; z < limit; z++) {
            numberStore.push_back(atoi(marksArray[readBuffer + z].c_str()));
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }

    //Sort the vector
    std::sort(numberStore.begin(), numberStore.end());

    // Return the first element in the sorted vector (smallest) //
    return numberStore.front();
}

double findMaximum(const int arrayItemCount, std::vector<std::string>marksArray) {
    std::vector<int> numberStore;                           // Temp storage of numerical test data

    //Goes through the string vector and places the test scores into a int vector
    for (int i = 0; i < arrayItemCount; i++) {
        int limit = (atoi(marksArray[i].c_str()));  // Gets the string within the array and converts it to an integer
        int readBuffer = i + 2;                     // Skips the first two array values before the mark data
        int skipMarkData = limit + 1;

        // Loops through test score & stores them in a int vector array.
        for (int z = 0; z < limit; z++) {
            numberStore.push_back(atoi(marksArray[readBuffer + z].c_str()));
        }
        i = i + skipMarkData;                       // Skips the marks data and places (i) at the start of the next student
    }

    //Sort the vector
    std::sort(numberStore.begin(), numberStore.end());

    // Return the last element in the sorted vector (largest) //
    return numberStore.back();
}

int main() {
    //Load the file data into the array initially
    std::vector<std::string> marksArray;
    std::string fileName = "marks.txt";
    bool readFileSuccess = false;

    //Checks if read file was successful
    while (readFileSuccess == false)
    {
        readFileSuccess = readFile(fileName, marksArray);
        // Only display message if readFile is false, stops this message from displaying when process successful.
        if (readFileSuccess == false) {
            std::cout << "\nPress any key to retry!";
            std::cin.ignore(256, '\n');             //Stops the program from skipping ahead
            std::cin.get();                     //Pauses program
        }
    }

    //Get the final size of the array
    int arrayItemCount = marksArray.size();

    while (true) {
        int userInput;
        std::string studentID;

        //Related to case 7:
        char choice = 'n';                          // Y or N to exit data entry process                            
        std::string markInput;
        int amoutOfTestScores = 0;
        std::vector<std::string>addMarks;
        std::string intToStrOfTestScores;           //Converts int to string

        std::cout << "\n=================================\nStudent Score System \n";
        std::cout << "Menu\n"
            << "(1) Display marks\n"
            << "(2) Calculate mean\n"
            << "(3) Calculate median\n"
            << "(4) Find minimum\n"
            << "(5) Find maximum\n"
            << "(6) Find average of student\n"
            << "(7) Add new student data\n"
            << "(8) Quit program\n"
            << "Please enter a value between 1-8: ";
        std::cin >> userInput;

        switch (userInput) {
        case 1:
            std::cout << "=================================\n Display of Marks";
            displayMark(arrayItemCount, marksArray);
            break;
        case 2:
            std::cout << "=================================\nClass Mean\n=================================";
            std::cout << "\nThe class mean is: " << calculateMean(arrayItemCount, marksArray);
            break;
        case 3:
            std::cout << "=================================\nStudent Median\n=================================";
            std::cout << "\nThe class median is :" << calculateMedian(arrayItemCount, marksArray);
            break;
        case 4:
            std::cout << "=================================\nClass Minimum\n=================================";
            std::cout << "\nThe class minimum is :" << findMinimum(arrayItemCount, marksArray);
            break;
        case 5:
            std::cout << "=================================\nClass Maximum\n=================================";
            std::cout << "\nThe class maximum is :" << findMaximum(arrayItemCount, marksArray);
            break;
        case 6:
            std::cout << "=================================\nStudent Average\n=================================";
            std::cout << "\nPlease enter the student ID: ";
            std::cin >> studentID;
            std::cout << "\nThe average test scores for "
                      << studentID
                      << " are: "
                      << findAverageForStudent(arrayItemCount, marksArray, studentID)
                      << " %";
            break;
        case 7:
            std::cout << "\nPlease enter the Student ID:";
            std::cin >> studentID;
            addMarks.push_back(studentID);

            while (choice == 'n') {
                std::cout << "\nPlease enter a test score:";
                std::cin >> markInput;
                addMarks.push_back(markInput);

                //Counter of amount of test added
                ++amoutOfTestScores;

                std::cout << "Are you done? (y or n)?";
                std::cin >> choice;
            }

            //Inserts the total test items into the vector
            intToStrOfTestScores = std::to_string(amoutOfTestScores);   //Converts int to string
            addMarks.insert(addMarks.begin(), intToStrOfTestScores);

            //Calls the write function
            updateFile(fileName, addMarks);
            break;
        case 8:
            std::cout << "\nFor you may leave Narnia, but you shall never forget its existence - John Cena";
            exit(0);
            break;
        default:
            std::cout << "\nThe input is not valid. Please enter a number between 1-8";
            break;
        }
    }
    return 0;
}

I know it is long and tedious, but I wanted to know what you thought of the code.

I have incorporated advice from a code review I asked in the past ( such as using std::), but I feel as if my code is still sub-par and lacking in terms of efficiency and readability even though I achieved 100% for this particular assignment.

Answer 1

OK, let's start at the top. You need to choose good abstractions to make your code readable and maintainable. Right now, if you want to change anything about the problem, you have to change multiple places in the code. The details of the student file, for example, are spread all over the place.

C++ is (among other things) an object-oriented language. Let's use those object-oriented features to help us understand the code.

One of the fundamental types should be the Student. Instead of reading in a bunch of lines and making every function understand how those lines are formatted, let's write the code so that we read in a bunch of Students.

struct Student {
    std::string id;
    std::vector<int> test_scores;
};

std::istream& operator>>(std::istream& stream, Student& student) {
    int num_scores = 0;
    stream >> num_scores >> student.id;
    stream.test_scores.resize(num_scores);
    for (int& score : student.test_scores) {
        stream >> score;
    }
    return stream;
}

Now you can write readFile like this:

std::vector<Student> readFile(const std::string& filename) {
    std::vector<Student> students;
    std::ifstream file(filename);
    if (!fail) throw std::runtime_error("Could not open file.");
    std::copy(std::istream_iterator<Student>(file),
              std::istream_iterator<Student>(),
              std::back_inserter(students));
    return students;
}

There's a number of improvements here. First, as mentioned before, the rest of the functions don't have to know how to parse the file. Second, we're using istream's built-in parsing instead of calling out to atoi. We're not really doing much error checking here (but then again, neither were you). I'd say that the file format isn't very helpful here. If the first line of the file gave the total number of student records, you could throw an exception if file.fail() returned true after reading that number of records.

I'm not going to rewrite all your functions, but let's take a look at calculateMean as an example:

double calculateMean(const std::vector<Student>& students) {

Starting with the function signature: why should we copy the entire input to compute statistics? Let's pass by reference instead. Note how using a vector of Students allows us to omit arrayItemCount.

    double sum = 0.0;
    int num_scores = 0;
    for (const Student& student : students) {
        sum = std::accumulate(student.test_scores.begin(),
                              student.test_scores.end(),
                              sum);
        num_scores += student.test_scores.size();
    }
    return sum / num_scores;
}

Let's save on memory by a different choice of algorithm as well. Instead of copying all the test scores into a single array, add them up where they already are.

There's more to work on, but I think this provides some good places to start. A final note -- some of your code makes me think that you're working from an old version of C++ (e.g. you passed filename.c_str() to the ifstream constructor, you're not using range-based for loops). C++11 (released in 2011) improved the language significantly. Most compilers should support it by now. If you're using g++ or clang++ and they don't seem to support C++11 features, you can try passing -std=c++11 to the compiler.

Answer 2

I don't see anything apparently wrong with the coding, other than the selection of your index variables seems to be random (i in the outer loop, z in the inner one for example). i is pretty apparent as to its meaning (index), but things like z don't really have any semantic meaning. Like other variables, you are free to choose one that best describes its purpose and loop variables can have more than one character.

The other thing I see is that at the top of your file you have this:

*/
Variables Used:
------------------
int limit                   // States the amount of test data per student 
int readBuffer              // Skips the first two array values before the mark data
int skipMarkData            // Skips the marks data so that i is ready to start at the start of the next student
double sum                  // Totalling the sum of the student individual average
fileWriter                  // Used to write changes to file
fileReader                  // Used to read from file
string currentReadLine      // Reads the current line within the file
vector<int> numberStore     // Temp storage for string test data that is converted to integer for math processing
vector<string> marksArray   // Stores the contents of the file --> marks.txt
char choice                 // Stores users choice in regards to whether they want to continue data entry or not
string markInput            // Stores the user test data inputs as string for 
int amountOfTestScores      // Records the amount of test data the user has currently inputted
vector<string>addMarks      // Stores user data entry input
string intToStrOfTestScores // Converts the amountOfTestScores integer value into a string value for inclusion into addMarks array vector.
string studentID            // Stores the studentID value for lookup & data entry
int userInput               // Stores the menu selection values 1-8

*/

I would advise to not have a list of "variables used" in your program. They really don't serve any purpose (pretty sure you can figure out when reading the code that fileWriter writes to a file, etc). This only serves to add complexity to your code documentation. The problem is that if you change your variable name in your method, then you have to find it in there and change it too. Some automated renaming tools are good at renaming variables, but don't (usually) find it in documentation unless it follows a specific format (like Xmldoc).

The other thing that I see is that you have a lot of "thinking" in the code in the form of comments:

//Opens the file
fileReader.open(fileName.c_str());

Would you expect it to do anything else other than open the file?

//Reads the file and stores into array line by line
while (getline(fileReader, currentReadLine)) {
    marksArray.push_back(currentReadLine);
}

Again, the code explains itself, get the line from the file reader, store it in the currentReadLine variable, and push it into the marksArray vector. I find that comments should only explain complicated processes or other non-apparent issues. Don't document the lines by writing out what they do in comments.

Really, other than documentation above each method (which should conform to some formal spec like Xmldoc), you should avoid comments inside the code as much as possible. Here is a good article from VisualStudio Magazine about why comments are bad. Here is another article from SE member Jeff Atwood.