1
\$\begingroup\$

Here is my code that I made for a school assignment (a work in progress).

I'm a beginner at programming and I'll be glad if someone gives me a thorough review of my code. I've just switched from TurboC++ to Code::Blocks IDE using MinGW.

#include <fstream>
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <process.h>
#include <string.h>

using namespace std;

class Login
{
    public:
    char username[10], password[10];

}log1,log2;

int login()
{
    cout << "\t\t\t Login Panel" << endl;
    getchar();
    cout << "Enter username: ";
    cin.getline(log1.username,10);
    cout << "Enter password: ";
    cin.getline(log1.password,10);
    ifstream fin ("db", ios::binary | ios::in);
    if (!fin)
    {
        cout<< "Database cannot be accessed" << endl;
        return 0;
    }
    int i=0;
    while(!fin.eof())
    {
        fin.read((char*)&log2 , sizeof(Login));
        if (strcmp(log1.username,log2.username)==0 && strcmp(log1.password,log2.password)==0)
        {
            cout << "Logged in" << endl;
            i=1;
            break;
        }

    }
    fin.close();
    if(!i)
        cout << "Wrong Credentials";
    return i;
}
void signup()
{
    cout << "\t\t\t Signup Panel" << endl;
    cout << "Enter username: ";
    cin.getline(log1.username,10);
    cout << "Enter password: ";
    cin.getline(log1.password,10);
    if( !strlen(log1.username) || !strlen(log1.password) )
    {
        cout << "Error, unsupported characters" << endl;
        signup();
    }
    ifstream fin ("db", ios::binary | ios::in);
    while(!fin.eof())
    {
        fin.read((char*)&log2 , sizeof(Login));
        if (!fin)
            break;
        if (strcmp(log1.username,log2.username)==0 )
        {
            cout << "User already exists!" << endl;
            system("PAUSE");
            system("CLS");
            signup();
        }

    }
    fin.close();
    ofstream fout ("db", ios::binary | ios::app);
    fout.write((char*)&log1 , sizeof(Login));
    fout.close();
}
class stu
{
    uint32_t rollno;
    char Name[20];
    char Class[4];
    float marks;
    char grade;
    char calcgrade(float t1, char t2)
    {
        if (t1 >= 75) t2='A';
        else if (t1 >= 60) t2 = 'B';
        else if (t1 >= 50) t2 = 'C';
        else if (t1 >= 40) t2 = 'D';
        else t2 = 'E' ;
        return t2;
    }

  public:
    void getdata()
    {
        cout<<"Rollno :";   cin>>rollno;
        cout<<"Class :";    cin>>Class;
        cout<<"Name :";     getchar();  cin.getline(Name, 20);
        cout<<"Marks :";    cin>>marks;
        grade=calcgrade(marks,grade);
    }
    void putdata()
    {
        cout<< "Rollno : " << rollno << "\t Name : " << Name
            << "\n Marks : " << marks << "\t Grade : " << grade << endl;
    }
    int getrno() {  return rollno;    }
} s1, stud ;

int data_append(char* neim)
{
    ifstream fi (neim, ios::in | ios::binary);
    if (!fi)
    {
        return -1;
    }
    ofstream fo ("temp.dat", ios::out | ios::binary);
    char last ='y';
    std::cout << " Enter details of student whose record is to be inserted \n ";
    s1.getdata();
    while (!fi.eof())
    {
        fi.read((char*)&stud, sizeof(stu));
        if ( s1.getrno()<= stud.getrno())
        {
            fo.write((char*)&s1, sizeof(stu));
            last = 'n';
            break;
        }
        else
            fo.write((char*)&stud, sizeof(stu));
    }
    if (last == 'y')
        fo.write((char*)&s1, sizeof(stu));
    else if (!fi.eof())
    {
        while (!fi.eof())
        {
        fi.read((char*)&stud, sizeof(stu));
        fo.write((char*)&stud, sizeof(stu));
        }
    }
    fi.close(); fo.close();
    remove(neim);
    rename("temp.dat",neim);
    return 0;
}

int data_delete(char* neim)
{
    ifstream fi (neim, ios::in | ios::binary);
    if (!fi)
    {
        cout << "No such file in database" << endl;
        system("PAUSE");
        return -1;

    }
    ofstream file ("temp.dat", ios::out | ios::binary);
    int rno;    char found = 'f' , confirm = 'n' ;
    cout << " Enter rollno of student whose record is to be deleted \n";
    cin >> rno;
    while (!fi.eof())
    {
        fi.read((char*)&s1,sizeof(stu));
        if ( s1.getrno() == rno )
        {
            s1.putdata();
            found = 't';
            cout << " Are you sure, you want to delete this record? (y/n).. ";
            cin>> confirm ;
            if (confirm == 'n')
                file.write((char*)&s1,sizeof(stu));
        }
        else
            file.write((char*)&s1,sizeof(stu));
    }
    if ( found == 'f' )
        cout << " Record not found ;__; \n";
    fi.close();    file.close();
    remove(neim);  rename("temp.dat",neim);
    return 0;
}

int data_modify(char* neim)
{
    fstream fio(neim,ios::in|ios::out|ios::binary);
    int rno ;   long pos ;  char found = 'f';
    cout << " Enter rollno of student whose record is to be modified \n";
    getchar();
    cin >> rno;
    while ( !fio.eof())
    {
        pos = fio.tellg();
        fio.read((char*)&s1, sizeof(stu));
        if ( s1.getrno() == rno )
        {
            fio.seekg(pos);
            fio.write((char*)&s1, sizeof(stu));
            found = 't';
            break;
        }
    }
        if ( found == 'f')
            return -1;
        fio.seekg(0);
        cout << "Now the file contains \n";
        while(!fio.eof())
        {
            fio.read((char*)&stud, sizeof(stu));
            stud.putdata();
        }
        fio.close();
        return 0;
}

int data_search(char* neim)
{

    ifstream fi (neim, ios::in | ios::binary);
    if (!fi)
    {
        return -1;

    }
    int rno;    char found = 'f' ;
    cout << " Enter rollno of student whose record is to be searched \n";
    cin >> rno;
    while (!fi.eof())
    {
        fi.read((char*)&s1,sizeof(stu));
        if ( s1.getrno() == rno )
        {
            s1.putdata();
            found = 't';
            fi.close();
        }
    }
    if ( found == 'f' )
        return -2;
    return 0;
}
int data_new(char* Class)
{
    ifstream tmp1 (Class,ios::in);
    if (tmp1)
    {
        cout << "Class already exists!!!" << endl;
        tmp1.close();
        return -1;
    }
    ofstream newclass (Class,ios::out);
    newclass.close();
    return 0;
}
int data_remove(char* ClassDel)
{
    ifstream tmp1 (ClassDel,ios::in);
    if ( !tmp1 )
        return -1;
    tmp1.close();
    remove(ClassDel);
    return 0;
}
int main()
{
  loginpanel:
    if(!login())
    {
        system("pause");
        signup();
    }
    system("pause");
    menu:
    system("cls");
    cout << "\t\t\t Enter the number to proceed to corresponding operation" << endl;
    cout << "1. Create Class" << endl
         << "2. Append Data" << endl
         << "3. Delete Data" << endl
         << "4. Modify Data" << endl
         << "5. Search Record" << endl
         << "6. Delete Class" << endl
         << "7. Logout" << endl
         << "8. Exit" << endl;
    int *op = new int;
    cin >> *op;
    switch (*op)
    {
        case 1 : {  char Class[10];
                    cout << "Enter new class name :";
                    getchar();
                    cin.getline(Class,10);
                    cout << "\nCreating Class files.." << endl;
                    *op=data_new(Class);
                    if (*op==(-1))
                    {
                        cout << "Class already exists!!!" << endl;
                        break;
                    }
                    cout << "Class creation successful" << endl;
                    system("pause");
                    delete op;
                    break;
                 }
        case 2 : {
                 system("cls");
                 char neim[8];
                 cout << "Enter class (use numerals only)";      //implemented for school project
                 getchar();
                 cin.getline(neim,8);
                 *op=data_append(neim);
                 if (*op==(-1))
                 {
                    cout << "No such file in database" << endl;
                    break;
                 }
                 ifstream fi(neim, ios::in);
                 cout << "File now contains : \n";
                 while (!fi.eof())
                {
                    fi.read((char*)&stud, sizeof(stu));
                    if (fi.eof()) break;
                    stud.putdata();
                }
                fi.close();
                delete op;
                break;
                }
        case 3 : {
                  cout << "Enter class";      //implemented for school project
                  getchar();
                  char neim1[8];
                  cin.getline(neim1,8);
                  *op=data_delete(neim1);
                  if (*op==(-1))
                  {
                        cout << "No such file in database" << endl;
                        system("PAUSE");
                  }
                  ifstream fi;
                  fi.open(neim1, ios::in);
                  cout << "File now contains : \n";
                  while (!fi.eof())
                  {
                        fi.read((char*)&stud, sizeof(stu));
                        if (fi.eof()) break;
                        stud.putdata();
                  }
                  fi.close();
                    delete op;
                    break;
                 }
        case 4 : {
                    cout << "Enter class (use numerals only)";      //implemented for school project
                    char neim[8];
                    cin.getline(neim,8);
                    *op=data_modify(neim);
                    if ( *op==(-1))
                    {
                        cout << "Record not found ;__; \n";
                    }
                    delete op;
                    break;
                 }
        case 5 : {
                    cout << "Enter class (use numerals only)";      //implemented for school project
                    char neim[8];
                    cin.getline(neim,8);
                    *op=data_search(neim);
                    if(*op==(-1))
                    {
                        cout << "No such file in database" << endl;
                        system("PAUSE");
                    }
                    else if(*op==(-2))
                    {
                        cout << "Record not found" << endl;
                        system("PAUSE");
                    }
                    delete op;
                    break;
                 }
        case 6 : {
                    char ClassDel[10];
                    cout << "Enter class name to be deleted:" << endl;
                    cin.getline(ClassDel,10);
                    cout << "Deleting Class files.." << endl;
                    *op=data_remove(ClassDel);
                    if(*op==(-1))
                    {
                     cout << "No such class as '" << ClassDel << "'" << endl;
                    }
                    delete op;
                    break;
                 }
        case 7 : {
                    delete op;
                    goto loginpanel;
                 }
        case 8 : {
                    delete op;
                    exit(0);
                 }
        default : cout << "Wrong input!";
    }
    system("pause");
    goto menu;
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Nov 4 '18 at 15:44
  • \$\begingroup\$ You've used the object-oriented tag, but I don't see a class based approach. It's all global functions. Why did you pick it anyway? \$\endgroup\$ – Mast Nov 4 '18 at 15:46
  • \$\begingroup\$ @Mast sorry I didn't knew and won't update my code now onward, I know little about tags, I'll remove it \$\endgroup\$ – infinitEplus Nov 4 '18 at 18:18
0
\$\begingroup\$

using namespace std;

This is especially bad with your code because global variable log2 clashes with std::log2.

Don't use global variable

See this for some discussion.

Use plain int

int *op = new int;

Why is op an int*? It should just be an int.

Don't use goto

See this for discussion. It makes it more difficult to read your code.

More functions

At least every switch case should be a separate function with appropriate name.

Handle cin failure

You didn't check the status of cin after getline. If getline fail for example user typed in too many characters, cin get stuck in failed state and getline will not work as intended.

Weird recursion in setup

Setup called itself if it fails to read user name or password, or username already exist This coupled with previous point blows up the stack because getline will fail which result in username or password to be empty which then recursively call setup. Because log1 etc are global variable the recursive call can modify them half way in the previous function. Also the new instance of setup will run then the previous setup finish which duplicate the work that seems to be a bug.

Use better name

stu, s1, stud, op are poorly named. It is not clear what they meant to represent.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ op is an integer pointer just to demonstrate run time memory allocation using "new" operator. I can't understand how is it bad. Regarding {using namespace ..} I'll fix it. How can I jump to another place without goto? It's beyond my current knowledge. And regarding separate functions of each case, I wanted to give more control to the main() function. I'll learn about the cin failure and I'll improve my variable names. Thanks ✌🏿 \$\endgroup\$ – infinitEplus Oct 25 '18 at 9:29
  • \$\begingroup\$ @infinitEplus because 1) new "can fail" and you havent checked the case it failed to allocate memory, 2) its inefficient compared to int. 3) its more complex, add several lines to your code. 4) you leak memory in the default case because you forgot to free it. \$\endgroup\$ – wooooooooosh Oct 25 '18 at 22:53
  • \$\begingroup\$ @infinitEplus goto menu can be replaced by a loop, goto loginpanel can be replaced by a loop or some logic to handle logged out as special case in the main loop. \$\endgroup\$ – wooooooooosh Oct 25 '18 at 22:54
  • \$\begingroup\$ I updated the code in my git, removed recursions and using namespace std, but login() doesn't work, signup sucessfull, login always "Wrong Credentials". See my commit history if it helps \$\endgroup\$ – infinitEplus Nov 4 '18 at 18:24
0
\$\begingroup\$
#include <stdio.h>
#include <stdlib.h>
#include <process.h>
#include <string.h>

When you see an include directive followed by a vendor header, ending with.h, it's an alert: You're surely wrong. Michael Burr explain why in SO

using namespace std;

Don't. Or at least, try to avoid this line. Look at this SO thread to know why.


I know there will be many more things to say, but it's late. I'll edit my post tomorrow to add more tips.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Okay 🙂 waiting \$\endgroup\$ – infinitEplus Oct 27 '18 at 8:45

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.