1
\$\begingroup\$

I require feedback on the plethora of bad practices that are definitely existing in the following code:

#include<fstream.h>
#include<conio.h>
#include<stdio.h>
#include<process.h>
#include<string.h>
#include<iomanip.h>

//***************************************************************
//                   CLASSES USED IN PROJECT
//****************************************************************


class book{
    char bno[6];
    char bookName[50]; //title of book
    char authName[20]; //author of book
  public:
    void create_book(){
        cout<<"\nNEW BOOK ENTRY...\n";
        cout<< "\nEnter Book Number : ";
        cin>>bno;
        cout<< "\n\nEnter Name of the Book : ";
        gets(bookName);
        cout<<"\n\nEnter Author's name : ";
        gets(authName);
        cout<<"\n\n\nBook Created...";
    }

    void show_book(){
        cout<<"\nBook number : " << bno;
        cout<<"\nBook Name : ";
        puts(bookName);
        cout<<"Author Name : ";
        puts(authName);
    }    

    void modify_book(){
        cout<<"\nBook number : " << bno;
        cout<<"\nModify Book Name : ";
        gets(bookName);
        cout<<"\nModify Author's Name : ";
        gets(authName);
    }

    char* retbno(){ return bno; }

    void report(){cout<<bno<<setw(30)<<bookName<<setw(30)<<authName<<endl;}

}; //end of class book

class student{
 private:
    char admno[6]; //Admission number
    char name[20]; //Name of the student
    char stbno[6]; //Student Book number
    int token;
 public:
    void create_student(){
        clrscr();
        cout<<"\nNEW STUDENT ENTRY...\n";
        cout<<"\nEnter Admission number : ";
        cin>>admno;
        cout << "\n\nEnter Name of Student : ";
        gets(name);
        token = 0;
        stbno[0] = '/0';
        cout<<"\n\nStudent Record Created..";
    }

    void show_student(){
        cout<<"\nAdmission number : "<<admno;
        cout<<"\nStudent Name : ";
        puts(name);
        cout<<"\nNumber of Book(s) issued : "<<token;
        if( token == 1 ) cout<<"\nBook No "<<stbno;
    }

    void modify_student(){
        cout<<"\nAdmission number : "<<admno;
        cout<<"\nModify Student Name : ";
        gets(name);
    }

    char* retadmno(){ return admno ; }

    char* retstbno(){ return stbno ; }

    int rettoken(){ return token ; }

    void addtoken(){ token = 1; }

    void resettoken(){ token = 0; }

    void getstbno(char t[]){ strcpy(stbno,t); }

    void report(){cout <<"\t"<<admno<<setw(20)<<name<<setw(10)<<token<<endl;}

};//end of class student


//***************************************************************
// Global declaration for stream object, object
//****************************************************************

fstream f, f1;
book b;
student s;

//***************************************************************
// Functions to write record to file
//****************************************************************

void write_book(){
    char ch;
    f.open("book.dat", ios::out|ios::app);
    do{
        clrscr();
        b.create_book();
        f.write((char*)&b, sizeof(book));
        cout<<"\n\nDo you want to add more books...(y/n?)";
        cin>>ch;
    }while( ch == 'y'|| ch == 'Y' );
    f.close();
}

void write_student(){
    char ch;
    f.open("student.dat", ios::out|ios::app);
    do{
        s.create_student();
        f.write((char*)&st, sizeof(student));
        cout<<"\n\nDo you want to add more students...(y/n?)";
        cin>>ch;
    }while(ch=='y'||ch=='Y');
    f.close();
}


//***************************************************************
// Function to read specific record from file
//****************************************************************


void display_spb(char n[]){
    cout<<"\nBOOK DETAILS\n";
    int flag = 0;
    f.open("book.dat", ios::in);
    while(f.read((char*)&b, sizeof(book))){
        if( strcmpi(b.retbno(),n) == 0 ){
            b.show_book();
            flag = 1;
        }
    }

    f.close();
    if(flag==0) cout<<"\n\nBook does not exist";
    getch();
}

void display_sps(char n[]){
    cout<<"\nSTUDENT DETAILS\n";
    int flag = 0;
    f.open("student.dat", ios::in);
    while(f.read((char*)&s, sizeof(student))){
        if((strcmpi(s.retadmno(),n)==0)){
            s.show_student();
            flag = 1;
        }
    }
    f.close();
    if( flag == 0 ) cout<<"\n\nStudent does not exist";
    getch();
}


//***************************************************************
// Functions to modify records of file
//****************************************************************


void modify_book(){
    char n[6];
    int found = 0;
    clrscr();
    cout<<"\n\n\tMODIFY BOOK REOCORD.... ";
    cout<<"\n\n\tEnter The book no. of The book";
    cin>>n;
    f.open("book.dat", ios::in|ios::out);
    while(f.read((char*)&b, sizeof(book)) && found == 0){
        if(strcmpi(b.retbno(),n) == 0){
            b.show_book();
            cout<<"\nEnter New Details of Book"<<endl;
            b.modify_book();
            int pos = -1 * sizeof(b);
            f.seekp(pos,ios::cur);
            f.write((char*)&b, sizeof(book));
            cout<<"\n\n\t Record Updated";
            found = 1;
        }
    }

    f.close();
    if( found == 0 ) cout<<"\n\n Record Not Found ";
    getch();
}


void modify_student(){
    char n[6];
    int found = 0;
    clrscr();
    cout<<"\n\n\tMODIFY STUDENT RECORD... ";
    cout<<"\n\n\tEnter Admission number of Student";
    cin >> n;
    f.open("student.dat", ios::in|ios::out);
    while( f.read((char*)&s, sizeof(student)) && found == 0 ){
        if(strcmpi(s.retadmno(),n) == 0){
            s.show_student();
            cout<<"\nEnter The New Details of student"<<endl;
            s.modify_student();
            int pos = -1 * sizeof(s);
            f.seekp(pos, ios::cur);
            f.write((char*)&s, sizeof(student));
            cout<<"\n\n\t Record Updated";
            found = 1;
        }
    }
    f.close();
    if( found == 0 ) cout<<"\n\n Record Not Found ";
    getch();
}

//***************************************************************
// Function to Delete record of file
//****************************************************************

void delete_student(){
    char n[6];
    int flag = 0;   
    clrscr();
    cout<<"\n\n\n\tDELETE STUDENT...";
    cout<<"\n\nEnter The admission no. of the Student You Want To Delete : ";
    cin>>n;
    f.open("student.dat", ios::in|ios::out);
    fstream f2;
    f2.open("Temp.dat", ios::out);
    f.seekg(0, ios::beg);
    while(f.read((char*)&s, sizeof(student))){
        if(strcmpi(s.retadmno(), n) != 0) f2.write((char*)&s, sizeof(student));
        else flag = 1;
    }

    f2.close();
        f.close();
        remove("student.dat");
        rename("Temp.dat", "student.dat");
        if( flag == 1 ) cout<<"\n\n\tRecord Deleted ..";
        else cout<<"\n\nRecord not found";
        getch();
}


void delete_book(){
    char n[6];
    clrscr();
    cout<<"\n\n\n\tDELETE BOOK ...";
    cout<<"\n\nEnter Book number of Book to Delete : ";
    cin >> n;
    f.open("book.dat", ios::in|ios::out);
    fstream f2;
    f2.open("Temp.dat", ios::out);
    f.seekg(0, ios::beg);
    while( f.read((char*)&b, sizeof(book)) ) if(strcmpi(b.retbno(),n)!=0)                          f2.write((char*)&b, sizeof(book));
    f2.close();
    f.close();
    remove("book.dat");
    rename("Temp.dat","book.dat");
    cout<<"\n\n\tRecord Deleted ..";
    getch();
}



//***************************************************************
// Function to display all students list
//****************************************************************

void display_alls(){
    clrscr();
        f.open("student.dat",ios::in);
        if(!f){
            cout<<"ERROR!!! FILE COULD NOT BE OPEN ";
            getch();
            return;
        }

    cout<<"\n\n\t\tSTUDENT LIST\n\n";
    cout<<"==================================================================\n";
    cout<<"\tAdmission No."<<setw(10)<<"Name"<<setw(20)<<"Book Issued\n";
    cout<<"==================================================================\n";

    while(f.read((char*)&s, sizeof(student))) s.report();

    f.close();
    getch();
}


//***************************************************************
// Function to display Books list
//****************************************************************

void display_allb(){
    clrscr();
    f.open("book.dat", ios::in);
    if(!f){
        cout<<"ERROR!!! FILE COULD NOT BE OPEN ";
        getch();
        return;
    }
    cout<<"\n\n\t\tBook LIST\n\n";
    cout<<"=========================================================================\n";
    cout<<"Book Number"<<setw(20)<<"Book Name"<<setw(25)<<"Author\n";
    cout<<"=========================================================================\n";
    while(f.read((char*)&b, sizeof(book))) b.report();
     f.close();
     getch();
}



//***************************************************************
//      function to issue book
//****************************************************************

void book_issue(){
    char sn[6], bn[6];
    int found = 0, flag = 0;
    clrscr();
    cout<<"\n\nBOOK ISSUE ...";
    cout<<"\n\n\tEnter Student's Admission number : ";
    cin>>sn;
    f.open("student.dat", ios::in|ios::out);
    f1.open("book.dat", ios::in|ios::out);
    while(f.read((char*)&s, sizeof(student)) && found == 0){
        if(strcmpi(s.retadmno(), sn) == 0){
            found = 1;
            if(s.rettoken() == 0){
                cout<<"\n\n\tEnter Book number : ";
                cin>>bn;
                while(f1.read((char*)&b, sizeof(book)) && flag==0){
                    if(strcmpi(b.retbno(), bn) == 0){
                        b.show_book();
                        flag = 1;
                        s.addtoken();
                        s.getstbno(b.retbno());
                        int pos = -1 * sizeof(s);
                        f.seekp(pos, ios::cur);
                        f.write((char*)&s, sizeof(student));
                        cout<<"\n\n\t Book issued successfully\n\nPlease Note: Write the current date in backside of your book \n and submit within 15 days fine Rs. 1 for each day \n after 15 days period";
                    }
                }
                if( flag == 0 ) cout<<"Book number does not exist";
            }
            else cout<<"You have not returned the last book ";

        }
    }
    if(found == 0) cout<<"Student record not exist...";
    getch();
    f.close();
    f1.close();
}

//***************************************************************
// Function to deposit book
//****************************************************************

void book_deposit()
{
    char sn[6],bn[6];
    int found = 0, flag = 0, day, fine;
    clrscr();
    cout<<"\n\nBOOK DEPOSIT ...";
    cout<<"\n\n\tEnter Student’s Admission number : ";
    cin>>sn;
    f.open("student.dat", ios::in|ios::out);
    f1.open("book.dat", ios::in|ios::out);
    while(f.read((char*)&s, sizeof(student)) && found == 0){
        if(strcmpi(s.retadmno(), sn)==0){
            found = 1;
            if(s.rettoken() == 1){
                while(f1.read((char*)&b, sizeof(book))&& flag==0){
                    if(strcmpi(b.retbno(), s.retstbno())==0){
                        b.show_book();
                        flag = 1;
                        cout<<"\n\nBook deposited in no. of days : ";
                        cin>>day;
                        if(day > 15){
                            fine = (day-15) * 1;
                            cout<<"\n\nFine to deposited is Rs. "<<fine;
                        }
                        s.resettoken();
                        int pos = -1 * sizeof(s);
                        f.seekp(pos,ios::cur);
                        f.write((char*)&s, sizeof(student));
                        cout<<"\n\n\t Book deposited successfully";
                    }
                }
                if(flag==0)cout<<"Book no does not exist";
            }
            else cout<<"No book is issued..please check!!";
        }
       }
    if(found==0) cout<<"Student record not exist...";
    getch();
    f.close();
    f1.close();
  }

//***************************************************************
//      INTRODUCTION FUNCTION
//****************************************************************

void intro(){
    clrscr();
    gotoxy(35,11);
    cout<<"LIBRARY";
    gotoxy(35,14);
    cout<<"MANAGEMENT";
    gotoxy(35,17);
    cout<<"SYSTEM";
    cout<<"\n\nMADE BY : Nikhil Kartha";
    getch();
}

//***************************************************************
//      ADMINISTRATOR MENU FUNCTION
//****************************************************************

void admin_menu(){
    clrscr();
    int ch2;
    cout<<"\n\n\n\tADMINISTRATOR MENU";
    cout<<"\n\n\t(1) CREATE STUDENT RECORD";
    cout<<"\n\n\t(2) DISPLAY ALL STUDENTS RECORD";
    cout<<"\n\n\t(3) DISPLAY SPECIFIC STUDENT RECORD ";
    cout<<"\n\n\t(4) MODIFY STUDENT RECORD";
    cout<<"\n\n\t(5) DELETE STUDENT RECORD";
    cout<<"\n\n\t(6) CREATE BOOK ";
    cout<<"\n\n\t(7) DISPLAY ALL BOOKS ";
    cout<<"\n\n\t(8) DISPLAY SPECIFIC BOOK ";
    cout<<"\n\n\t(9) MODIFY BOOK ";
    cout<<"\n\n\t(10) DELETE BOOK ";
    cout<<"\n\n\t(11) BACK TO MAIN MENU";
    cout<<"\n\n\tPlease Enter Your Choice (1-11) ";
    cin>>ch2;
    switch(ch2){
        case 1: 
            clrscr();
            write_student();
            break;
        case 2: 
            display_alls();
            break;
        case 3:
            char num[6];
            clrscr();
            cout<<"\n\n\tPlease Enter The Admission No. ";
            cin>>num;
            display_sps(num);
            break;
        case 4: 
            modify_student(); 
            break;
        case 5: 
            delete_student(); 
            break;
        case 6: 
            clrscr();
            write_book(); 
            break;
        case 7: 
            display_allb(); 
            break;
        case 8:
            char num[6];
            clrscr();
            cout<<"\n\n\tPlease Enter The book No. ";
            cin>>num;
            display_spb(num);
            break;
        case 9: 
            modify_book(); 
            break;
        case 10: 
            delete_book(); 
            break;
        case 11: return;
        default: cout<<"\a";
    }
    admin_menu(); //recursion
}


//***************************************************************
//      THE MAIN FUNCTION OF PROGRAM
//****************************************************************

void main(){
    char ch;
    intro();
    do{
        clrscr();
        cout<<"\n\n\n\tMAIN MENU";
        cout<<"\n\n\t(1) BOOK ISSUE";
        cout<<"\n\n\t(2) BOOK DEPOSIT";
        cout<<"\n\n\t(3) ADMINISTRATOR MENU";
        cout<<"\n\n\t(4) EXIT";
        cout<<"\n\n\tPlease Select Your Option (1-4) ";
        ch = getche();
        switch(ch){
            case '1':
                clrscr();
                book_issue();
                break;
            case '2': 
                book_deposit();
                break;
            case '3': 
                admin_menu();
                break;
            case '4': exit(0);
            default : cout<<"\a";
        }//end of switch
    }while(ch!='4');
}

//***************************************************************
//              END OF PROJECT
//***************************************************************

This code was written in Turbo C++.

\$\endgroup\$
5
\$\begingroup\$

Some feedback:

  1. replace the char xxx[n] declarations with std::string
  2. use standard headers:
    • fstream instead of fstream.h
    • conio.h should not be used (non-standard and dated)
    • stdio.h should be replaced by iostream
    • string.h functionality should be replaced by string header functionality
    • iomanip.h should be iomanip
  3. use better identifiers:
    • the only true thing the name retbno is telling me is "returning a bno" (what is a bno?); (to understand why it's a bad name, this is about as useful as it would be to rename void book::create_book() into void book::retvoid()).
    • use consistent identifiers: if you use snake_case, then use it everywhere - either have book::create_book, book::show_book, ..., book::ret_bno, or have book::createbook, book::showbook, ..., book::retbno (or something else) but not both conventions at the same time.
  4. do not use gets and puts (use std::getline and std::cout instead)
  5. the code comments you use are mostly useless (comments should add details on "the why" of things, not on "the what").

    Example of bad comment:

    //***************************************************************
    //                   CLASSES USED IN PROJECT
    //****************************************************************
    

    (because when we see the class declaration, we know it's a class used in the project)

    Example good comments (and where you should have them):

    char bno[6];
    char bookName[50]; //title of book
    char authName[20]; //author of book
    

    (bno is probably the only one that should be commented; the other two are obvious)

  6. use dependency injection for std::cin and std::cout.

    Example old code:

    void create_book() {
        cout<<"\nNEW BOOK ENTRY...\n";
        cout<< "\nEnter Book Number : ";
        cin>>bno;
        cout<< "\n\nEnter Name of the Book : ";
        gets(bookName);
        cout<<"\n\nEnter Author's name : ";
        gets(authName);
        cout<<"\n\n\nBook Created...";
    }
    

    Example new code:

    void create_book(std::ostream& out, std::istream& in) {
        out<<"\nNEW BOOK ENTRY...\n";
        out<< "\nEnter Book Number : ";
        in>>bno;
        out<< "\n\nEnter Name of the Book : ";
        std::getline(in, bookName); // bookName should be std::string
        out<<"\n\nEnter Author's name : ";
        std::getline(in, authName); // authName should be std::string
        out<<"\n\n\nBook Created...";
    }
    

    This will allow you to test the code with predefined inputs (see std::istringstream).

  7. If you can avoid having global variables, you probably should. That means, declare f, f1, b and s globals in the functions that use them. You should also have comprehensible names for your variables.

  8. You should not use C-style casts in C++. Criteria:

    • If you have a cast, refactor code until the cast is remvoed
    • If you cannot refactor the code, use a C++-style cast
    • The only reason you should have C-style casts in your code is if you are working on legacy code and the casts are too many to remove with the effort you can reasonably make.
  9. Your use of fstream is not idiomatic.

    Your use:

    fstream f, f1;   /// <--------------
    // ...
    
    void write_book(){
        char ch;
        f.open("book.dat", ios::out|ios::app);   /// <--------------
        // ...
        f.close();   /// <--------------
    }
    

    Idiomatic use:

    void write_book(){
        char ch;
        std::ifstream in("book.dat", std::ios::out|std::ios::app);   /// <-- local var.
        // ...
        /// <-------------- no explicit call to close necessary
    }
    
  10. If you remove the calls to clrscr (it's not standard anyway), you can also remove the calls to getch (because their purpose is to keep the data visible before the next clrscr call).

  11. consider having at most one instruction per line.

    For example, this code is difficult to read:

    while(f.read((char*)&s, sizeof(student))) s.report();
    
  12. Normally, you should use the most specialized type for a job. In void book_issue(), found and flag should be declared as bool.

  13. void admin_menu() uses a big, monolythic switch. Consider replacing it with a dispatch map (extract cases into separate functions, then create a std::map<int, void(void)> to map these functions to separate inputs).

  14. You should not have the entire code in a single file. Instead, you should have each class in a pair of files (.h for the declaration and .cpp for the definition). This would allow you to remove the huge separation blocks.

    Old code:

    #include<fstream.h>
    // ...
    #include<iomanip.h>
    
    //***************************************************************
    //                   CLASSES USED IN PROJECT
    //****************************************************************
    
    // ...
    
    //***************************************************************
    // Global declaration for stream object, object
    //****************************************************************
    
    // ...
    
    //***************************************************************
    // Functions to write record to file
    //****************************************************************
    
    // ...
    

New code:

    #include<fstream.h>
    // ...
    #include<iomanip.h>

    #include "book.h"
    #include "student.h"

    // ...
\$\endgroup\$
  • 1
    \$\begingroup\$ I'd argue that a variable named bookTitle is better than a comment stating "title of book". When we have descriptive variable names, every use of the variable tells us what that variable is, and a comment explaining what the variable is becomes quite redundant. \$\endgroup\$ – nhgrif Nov 3 '14 at 11:31
  • 1
    \$\begingroup\$ I agree. My point was not about choosing between good names, and commented variables. Infact, I explicitly pointed out that bookName and authName should probably not be commented, because they have good names. \$\endgroup\$ – utnapistim Nov 3 '14 at 12:04
2
\$\begingroup\$

The code is very long and would take some time to review it completely. However, following things needs to be updated on immediate basis:

  1. Replace all char types (example: char bookName[50]) to std::string.
  2. Use std::getline instead of gets.
  3. Factor the code/class in .h and .cpp files. This would modularize your code.
  4. include the standard C++ header file without their extension (example: #include <fstream>)
  5. Use the std and other appropriate namespace in your code (example: std::cout instead of cout).
  6. Use the std::string instead of C-style string manipulation. This is applicable to your entire program.

These would avoid many problems in your code (overflow and related problems). You can actually compile your code with any modern online C++ compiler (GCC/VS2013) and you would get many warnings/errors in your code. You should start fixing those first and then incorporate the above suggestions.

For recommended practice/suggestion you may want to follow the ISOCpp FAQ section. They contain excellent information with code snippet which would be helpful for understanding any concepts.

Apart from these you may want to refer Bjarne Stroustrup introductory book A Tour of C++ on modern C++ language. It contains very concise and excellent information about modern C++.

\$\endgroup\$
  • \$\begingroup\$ If the code has errors, a vote to close should be cast or a comment posted... Certainly not an answer... \$\endgroup\$ – nhgrif Nov 3 '14 at 11:33
  • \$\begingroup\$ Apologies but this code is intended to be run in Turbo C++ ; which does not see any errors with the code. I do not intend to modernise it (yet) \$\endgroup\$ – Nick Nov 3 '14 at 11:48
  • \$\begingroup\$ I will someday, but currently I'm more concerned about more fundamental stupidities :D Thank you very much for your review. Your points have been duly noted :D \$\endgroup\$ – Nick Nov 3 '14 at 12:02
  • 2
    \$\begingroup\$ @nhgrif: Error/Warning in my post meant to be some support provided in modern C++ compilers in finding/helping out the bad uses. Example like VS gives _CRT_SECURE_NO_WARNINGS waring and hence the code will not compile when program uses c style functions strcpy. Otherwise the code posted by author is valid C++(old style) code. Hence I had provided my suggestion/answer. Hope it make sense to everyone. \$\endgroup\$ – Mantosh Kumar Nov 3 '14 at 12:41

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.