3
\$\begingroup\$

This is simple student information system project where you can do following things:

  1. Add Records
  2. List Records
  3. Modify Records
  4. Delete Records

To store data a .txt file is used.

I just want an honest critique of my code so I can improve.

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

using namespace std;

void flowcontrol();//directs you based on what you are trying to do;
int entrynumb();//counts the number astericks and stores them as a base for how many students they are for student number assignment;
void addrecord();//adds new students to the database;
void seerecord(string n);//lets you see the data on a student number;
void modifyrecord(string n, char c);//deletes a desired record;


int number = 0;
string strnumber = to_string(number);


struct student
{
    string firstname, lastname, birthday;
    int age, grade;
    int number;

    void writeto(student a)//writes student values to a file for retrieval and modification;
    {
        ofstream datastore;
        datastore.open ("datastore.txt", ios_base::app);        
        datastore   << '\n' << "Number : " <<   a.number;     
        datastore   << '\n' << "Name :"    << a.firstname << ' ' << a.lastname;
        datastore   << '\n' << "DOB : "    << a.birthday;
        datastore   << '\n' << "Age : "    <<      a.age;
        datastore   << '\n' << "Grade : "  <<    a.grade;
        datastore   << '\n' << "*";
        datastore   << '\n' << "----------------------------------------------";

        datastore.close();

    }

};


int main()
{

    flowcontrol();   
}

void spacer(int a)//adds a certain number of newline spaces;
{
    cout << string(a, '\n');
}

void flowcontrol()
{
    spacer(3);
    cout << "Welcome to the student database version 1.101" << '\n';
    cout << "<------------------------------------------->" << '\n';
    cout << "          What would you like to do?         " << '\n';
    cout << "         ADD | MODIFY | DELETE | SEE         " << '\n';
    cout << "          a  |    m   |   d    |   s         " << '\n';
    spacer(3);

    char ch; cin >> ch;

    spacer(22);

    switch(ch)
    {
        case 'a':
            addrecord();
            break;

        case 's':
        {
            spacer(2);
            cout << "Student ID number : " << '\n';            
            string n; cin >> n;
            spacer(5);
            seerecord(n);

            break;
        }


        case 'd':
        {
            spacer(2);
            cout << "Student ID number : " << '\n';
            string n; cin >> n;
            spacer(5);     
            modifyrecord(n, 'd');

            break;
        }


        case 'm':
        {
            spacer(2);
            cout << "Student ID number : " << '\n';
            string n; cin >> n;
            spacer(5);
            modifyrecord(n, 'm');

            break;
        }

    }


}

int entrynumb()//student number generator;
{
    string line;//stores a line of the datastore file here for reference;
    vector<string> liner;//stores the amount of entries;


    ifstream myfile ("datastore.txt");//opens the datastore file;
    if (myfile.is_open())
    {    
        while (getline(myfile,line))//grabs the lines in datastore and does something;
        {

            if(line == "*")//does this if the condition is met;
            {
                liner.push_back("*");//pushes the asterick to the liner vector;          
            }
        }

    myfile.close();
    }



    return liner.size();//returns the number of astericks stored wich is directley correlated with the number of students; 

}


void addrecord()//new student generator;
{

    student strnumber;//initiates new student entry as a student object;

    strnumber.number = entrynumb();//grabs the proper student number;

    string strcontainer;//to store string responses such as firstname/lastname;
    int intcontainer;//to store int responses such as age, dob, etc;

    cout << "First name? " << '\n';
    cin  >> strcontainer; strnumber.firstname = strcontainer;
    spacer(1);
    cout << "last name ? " << '\n';
    cin  >> strcontainer; strnumber.lastname = strcontainer;
    spacer(1);
    cout << "birthday ? " <<  '\n';   
    cin  >> strcontainer; strnumber.birthday = strcontainer;
    spacer(1);
    cout << "age ? "      <<  '\n';
    cin  >> intcontainer; strnumber.age      = intcontainer;
    spacer(1);
    cout << "grade ? "    <<  '\n';
    cin  >> intcontainer; strnumber.grade    = intcontainer;
    spacer(1);
    strnumber.writeto(strnumber);//calls to write the students data to datastore file;

    number++;//adds the number up by one per entry to keep track of number of students;

    main();//restarts the process;
}


void seerecord(string n)//takes the number provided and shows the student information;
{
    string line;
    ifstream myfile ("datastore.txt");//opens the datastore file;
    if (myfile.is_open())
    {    
        while (getline(myfile,line))//grabs the lines in datastore and does something;
        {

            if(line == "Number : " + n)//loops through and prints every line related associated with the number;
            {   cout << "                Student entry                " << '\n';
                cout << "<------------------------------------------->" << '\n';
                cout << '\n' << line << '\n';
                for(int i = 0; i < 5; i++ )
                {

                    getline(myfile, line);
                    cout  << line << '\n';                        
                }  

                cout << "<------------------------------------------->";  

            }
        }

    myfile.close();
    }

    flowcontrol();
}



void modifyrecord (string n, char c)//delete//modify a desired record;
{

    vector<string> datahold;//holds all the entries kept unchanged;

    ifstream myfile ("datastore.txt");//opens the file;
    string line;//for reference to each line;

    if (myfile.is_open())
    {
        while (getline(myfile, line))
        {

            if(line == "Number : " + n)//if the number that is to be deleted is found it's associated entries wont be stored;
            {

                switch(c)//checks to see if your deleting or modifying a student entry;
                {

                    case 'd'://delete;
                    {                            
                        for(int i = 0; i < 6; i++)
                        {
                            getline(myfile, line);                    
                            cout  << "DELETING: " << line << '\n';
                        }

                        flowcontrol();

                        break;
                    }

                    case 'm'://edit;
                    {
                        string entries[4] = {"Name : ", "DOB : ", "Age : ", "Grade : "};

                        datahold.push_back(line);
                        for(int j = 0; j < 4; j++)//loops through and edits each entry;
                        {                               

                            string strcontainer;
                            getline(myfile, line);


                            cout << "Currently: " << line << '\n' << "Edit ->"; cin >> strcontainer;
                            datahold.push_back(entries[j] + strcontainer);
                            cout << "---------------------------------------->" << '\n'; 

                        }

                        flowcontrol();

                        break;                                                    
                    } 

                }                         

            }

            else
            {
                datahold.push_back(line);//pushes                 entries that won't be edited or deleted;
            }

        }

    myfile.close();
    }

    ofstream myfilenew;
    myfilenew.open ("datastore.txt"); 

    for(unsigned int i = 0; i < datahold.size(); i++)//iterates through all the kept entries and rewrites them to the file;
    {
        myfilenew << datahold[i] << '\n';
    }

}
\$\endgroup\$
4
  • \$\begingroup\$ Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc. \$\endgroup\$ Dec 29 '18 at 2:27
  • \$\begingroup\$ Nothing in particular. I've never had any of my code reviewed, so I just wanted some feedback on anything that I should improve on. \$\endgroup\$
    – Vildjharta
    Dec 29 '18 at 2:31
  • \$\begingroup\$ It is important that you clarify in your question/title exactly how you would like your code to be reviewed. \$\endgroup\$ Dec 29 '18 at 2:33
  • \$\begingroup\$ Okay that makes sense. I was under the impression that everything from the logic of it to the design structure would be critiqued automatically. Moving forward Ill clarify exactly what I want critiqued. \$\endgroup\$
    – Vildjharta
    Dec 29 '18 at 2:42
5
\$\begingroup\$

Here are some observations that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Return something useful from the subroutines

Almost every single one of the routines is declared as returning void. Something is wrong there. For example, if addrecord fails for some reason such as invalid input data, it would be nice if the function returned a value indicating this fact.

Eliminate global variables

My rewrite of this code uses no global variables, so clearly they are neither faster nor necessary. Eliminating them allows your code to be more readable and maintainable, both of which are important characteristics of well-written code. Global variables introduce messy linkages that are difficult to spot and error prone.

Use better naming

The name flowcontrol is not really helpful, since it does much more than simply reading -- all of the processing, including output, is done there. Names like line are fine in context, but myfile is not such a good name. Variable and function names should tell the reader why they exist, ideally without further comments necessary. So instead of spacer() I'd probably name it print_empty_lines() for example.

Use string concatenation

The menu includes these lines:

std::cout << "Welcome to the student database version 1.101" << '\n';
std::cout << "<------------------------------------------->" << '\n';
std::cout << "          What would you like to do?         " << '\n';

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

std::cout 
    << "Welcome to the student database version 1.101\n"
    << "<------------------------------------------->\n" 
    << "          What would you like to do?         \n"
    // etc.

This reduces the entire menu to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Don't duplicate important constants

The filename is hardcoded right now (see next suggestion), but worse than that, it's done in five completely indpendent places. Better would be to create a constant:

static const char *FILENAME = "namelist.txt";

Consider the user

Instead of having a hardcoded filename, it might be nice to allow the user to control the name and location of the file. For this, it would make sense to use a command line argument and then pass the filename to the functions as needed.

Make better use of objects

The only member function of student is writeto which is not a good function name and may as well be a static function by the way it's written. Instead, I'd suggest something like this:

bool appendTo(const std::string &filename) const {
    std::ofstream datastore{"datastore.txt", std::ios_base::app};        
    if (datastore) {
        datastore   << 
            "Number : " << number     
            "\nName :"  << firstname << ' ' << lastname
            "\nDOB : "  << birthday
            "\nAge : "  << age
            "\nGrade : "<< grade
            "\n*"
            "\n----------------------------------------------\n";
    }
    bool result{datastore};
    return result;
}

Note that now it takes the filename as a parameter and return true if the write was successful. It is also const because it doesn't alter the underlying student data structure.

Understand the uniqueness of main

The end of flowcontrol() is this:

    main();//restarts the process;
}

However, that's not legal C++. You can't take the address of main nor call it. It may seem to work with your compiler but it's explicitly not guaranteed by the standard and is undefined behavior meaning that the program might do anything and that it's not predictable. If you need to repeat something, use a loop.

\$\endgroup\$
3
  • \$\begingroup\$ Is there a rule of thumb for when I should use void ? Or is it just better design to return values for error handling \$\endgroup\$
    – Vildjharta
    Dec 29 '18 at 19:45
  • 1
    \$\begingroup\$ Generally, if it doesn’t return something obvious like entrynumb and can’t fail (or throws an exception on failure) then void is ok. However if all of your functions return void it’s probably a sign that a little more thought about the design would be beneficial. \$\endgroup\$
    – Edward
    Dec 29 '18 at 19:56
  • \$\begingroup\$ Thank you for your feedback I will keep that in mind during my edit/moving forward! \$\endgroup\$
    – Vildjharta
    Dec 29 '18 at 20:00
4
\$\begingroup\$

These comments:

//directs you based on what you are trying to do;

don't need to end in semicolons. You're speaking English rather than C++ inside of them.

astericks

is spelled asterisks.

This code:

int number = 0;
string strnumber = to_string(number);

seems out-of-place. If it's just test code, delete it.

struct student

First, you'll probably want to capitalize Student. Your writeto method should be declared as

void writeto(ostream &datastore) const {

In particular, it shouldn't accept a, rather using this to identify the student; and it shouldn't open the file itself. It should be able to write to any stream (including cout).

For code like this:

char ch; cin >> ch;

Try to avoid adding multiple statements on one line, for legibility's sake.

A large section of your addrecord method should be moved to a method on Student. Specifically, this:

strnumber.number = entrynumb();//grabs the proper student number;

string strcontainer;//to store string responses such as firstname/lastname;
int intcontainer;//to store int responses such as age, dob, etc;

cout << "First name? " << '\n';
cin  >> strcontainer; strnumber.firstname = strcontainer;
spacer(1);
cout << "last name ? " << '\n';
cin  >> strcontainer; strnumber.lastname = strcontainer;
spacer(1);
cout << "birthday ? " <<  '\n';   
cin  >> strcontainer; strnumber.birthday = strcontainer;
spacer(1);
cout << "age ? "      <<  '\n';
cin  >> intcontainer; strnumber.age      = intcontainer;
spacer(1);
cout << "grade ? "    <<  '\n';
cin  >> intcontainer; strnumber.grade    = intcontainer;
spacer(1);

can be made into a method perhaps called initFromPrompt that initializes the current Student instance. Also, your "container" input pattern doesn't need to exist. For instance, for grade, simply

cin >> strnumber.grade;

This:

main();//restarts the process;

is misdesigned. You definitely don't want to be calling main yourself, for many reasons - including recursion. If you want to "restart the process", you should make some kind of top-level loop.

This:

ifstream myfile ("datastore.txt");//opens the file;
if (myfile.is_open())

is somewhat of an anti-pattern. This kind of code is going to lead to silent errors. Instead, read about how to enable std::ios::failbit so that you get exceptions from bad stream calls.

\$\endgroup\$
2
  • \$\begingroup\$ Can you give me an example of how I would call the write to method after your edit? I'm having a hard time understanding how it would work \$\endgroup\$
    – Vildjharta
    Dec 29 '18 at 19:51
  • 1
    \$\begingroup\$ strnumber.writeto(datastore), where datastore is your opened file and strnumber is your student instance \$\endgroup\$
    – Reinderien
    Dec 29 '18 at 23:40

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.