Text based student information system
This is simple student information system project where you can do following things:
- Add Records
- List Records
- Modify Records
- 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';
}
}
c++ c++11
New contributor
add a comment |
This is simple student information system project where you can do following things:
- Add Records
- List Records
- Modify Records
- 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';
}
}
c++ c++11
New contributor
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
add a comment |
This is simple student information system project where you can do following things:
- Add Records
- List Records
- Modify Records
- 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';
}
}
c++ c++11
New contributor
This is simple student information system project where you can do following things:
- Add Records
- List Records
- Modify Records
- 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';
}
}
c++ c++11
c++ c++11
New contributor
New contributor
edited 1 hour ago
Reinderien
3,035720
3,035720
New contributor
asked 3 hours ago
Vildjharta
62
62
New contributor
New contributor
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
add a comment |
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
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.
– Vildjharta
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
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.
– Vildjharta
2 hours ago
add a comment |
2 Answers
2
active
oldest
votes
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.
add a comment |
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.101n"
<< "<------------------------------------------->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.
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Vildjharta is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210535%2ftext-based-student-information-system%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
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.
add a comment |
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.
add a comment |
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.
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.
answered 1 hour ago
Reinderien
3,035720
3,035720
add a comment |
add a comment |
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.101n"
<< "<------------------------------------------->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.
add a comment |
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.101n"
<< "<------------------------------------------->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.
add a comment |
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.101n"
<< "<------------------------------------------->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.
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.101n"
<< "<------------------------------------------->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.
answered 1 hour ago
Edward
46.1k377209
46.1k377209
add a comment |
add a comment |
Vildjharta is a new contributor. Be nice, and check out our Code of Conduct.
Vildjharta is a new contributor. Be nice, and check out our Code of Conduct.
Vildjharta is a new contributor. Be nice, and check out our Code of Conduct.
Vildjharta is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f210535%2ftext-based-student-information-system%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Is there a particular aspect that you are seeking refinement with? Improved performance? Brevity? Etc.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago
It is important that you clarify in your question/title exactly how you would like your code to be reviewed.
– mickmackusa
2 hours ago
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.
– Vildjharta
2 hours ago