4
\$\begingroup\$

I have created a Library Management system. Can anyone please look at if see if there's any bad practices or if anything could be done better?

Main.cpp

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>
#include<stdio.h>
#include <string.h>
#include "Book.h"
#include "DVD.h"
#include "Student.h"

using namespace std;

int main() {
    Student student4;
    Book book2;
    DVD dvd2;

    while(1)
    {
        char mainSelect;
        char studentOption;
        char dvdOption;
        char bookOption;
        char name[30];

        bool processed = false;
        while(!processed)
        {
        // User Menu

        // Read user selection
        cin.getline( name, 80);
        mainSelect = name[0];

        // Switch statement to select between the options
        switch (mainSelect)
        {
          case '1':
            processed = true;
            break;
          case '2':
            processed = true;
            break;
          case '3':
            processed = true;
            break;
          case '4':
            processed = true;
            exit(0);
            break;
          case '5':
            default:
            cout<<"Incorrect selection. Please select from the given options." <<endl;
            processed = false;
            break;
        }
        };
        if (mainSelect == '1')
            // User Menu

                bool processed = false;
                while(!processed)
                {
                cin.getline( name, 80);
                dvdOption = name[0];

                switch(dvdOption)
                {
                    case '1':
                        dvd2.issueDVD();
                        processed = true;
                        break;
                    case '2':
                        dvd2.returnDVD();
                        processed = true;
                        break;
                    case '3':
                        dvd2.insertDVD();
                        processed = true;
                        break;
                    case '4':
                        dvd2.updateDVD();
                        processed = true;
                        break;
                    case '5':
                        dvd2.deleteDVD();
                        processed = true;
                        break;
                    case '6':
                        char barcode[6];
                        cout<<"Enter The DVD barcode: " <<endl;
                        cin>>barcode;
                        dvd2.searchDVD(barcode);
                        processed = true;
                        break;
                    case '7':
                        dvd2.showallDVDs();
                        processed = true;
                        break;
                    case '8':
                        processed = true;
                        break;
                    case '9':
                        exit(0);
                        break;
                    case '10':
                        default:
                        cout<<"Incorrect selection. Please select from the given options." <<endl;
                        processed = false;
                        break;
                }
                }
            }
            else if (mainSelect == '2')
            {
                // User Menu

                cin.getline(name, 80);
                bookOption = name[0];

                switch(bookOption)
                {
                    case '1':
                        book2.issueBook();
                        break;
                    case '2':
                        book2.returnBook();
                        break;
                    case '3':
                        book2.insertBook();
                        break;
                    case '4':
                        book2.updateBook();
                        break;
                    case '5':
                        book2.deleteBook();
                        break;
                    case '6':
                        char barcode[6];
                        cout<<"Enter The book barcode: " <<endl;
                        cin>>barcode;
                        book2.searchBook(barcode);
                        break;
                    case '7':
                        book2.showallBooks();
                        break;
                    case '8':
                        break;
                    case '9':
                        exit(0);
                        break;
                }
            }
            else if (mainSelect=='3')
            {
                // User Menu

                cin.getline( name, 80);
                studentOption = name[0];

                switch(studentOption){
                    case '1':
                        student4.insertStudent();
                        break;
                    case '2':
                        student4.showallStudents();
                        break;
                    case '3':
                        char regno[6];
                        cout<<"Enter the registration no. of the student you want to search: "<<endl;
                        cin>>regno;
                        student4.searchStudent(regno);
                        break;
                    case '4':
                        student4.updateStudent();
                        break;
                    case '5':
                        student4.deleteStudent();
                        break;
                    case '6':
                        exit(0);
                        break;
                    case '7':
                        cout << "Invalid selection!" << endl;
                        break;
                 }

              }

    return 0;
};

Media.h

#ifndef MEDIA_H_
#define MEDIA_H_

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>

class Media
{
public:
    char barcode[6];
    char name[50];
    char num[6];

    char* retBarcode()
        {
            return barcode;
        }
};



#endif /* MEDIA_H_ */

Book.h

#ifndef BOOK_H_
#define BOOK_H_

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>
#include "Media.h"

using namespace std;

class Book : public Media
{
    char author[20];

  public:
    void insertBook();
    void searchBook(char barcode[]);
    void updateBook();
    void deleteBook();
    void showallBooks();
    void issueBook();
    void returnBook();

    void newBook()
    {
        cout<<"Enter the Book Name: " <<endl;
        fgets(name, sizeof(name), stdin);
        cout<<"Enter the Name of Author: " <<endl;
        fgets(author, sizeof(author), stdin);
        cout<<"Enter the book barcode: " <<endl;
        cin>>barcode;
        cout<<"Book Added to the library" <<endl;
    }

    void displayBook()
    {
        cout<<"Enter the Book Name: " <<endl;
        puts(name);
        cout<<"Enter the Name of Author: ";
        puts(author);
        cout<<"Enter the Book barcode: " <<endl;
        cin>>barcode;
    }

    void editBook()
    {
        cout<<"Enter the Book Name you want to Update: " <<endl;
        cin>>barcode;
        cout<<"Enter New Book Name: " <<endl;
        fgets(name, sizeof(name), stdin);
        cout<<"Enter New Name of Author: " <<endl;
        fgets(author, sizeof(author), stdin);
    }

    void report()
    {
        cout<<"______________"<<" __________________"<<" _________________"<<endl;
        cout<<"|Name of Book| "<<"|Director of Book| "<<"|Barcode of Book| " <<endl;

        cout<<" "<<name<<"------------"<<author<<"-----------------"<<barcode<<endl;
    }


};         //class ends here


#endif /* BOOK_H_ */

Book.cpp

#include <stdlib.h>
#include <fstream>
#include <string.h>
#include <iostream>
#include<iomanip>
#include "Book.h"
#include "Student.h"

using namespace std;

Student student2;
Book book1;

void Book::insertBook()
{
    fstream file;
    file.open("book.dat",ios::out|ios::app);
    newBook();
    file.write((char*)&book1,sizeof(Book));
    file.close();
}

void Book::searchBook(char barcode[6])
{
    fstream file;
    int sys=0;
    file.open("book.dat",ios::in);
    while(file.read((char*)&book1,sizeof(Book)))
    {
        if(strcmp(retBarcode(),barcode)==0)
        {
            displayBook();
            sys=1;
        }
    }

    file.close();
    if(sys==0)
        cout<<"Error: No such Book found in System." <<endl;

}

void Book::updateBook()
{
    fstream file;
    int found=0;
    char barcode[6];
    cout<<"Enter barcode of the Book you want to update:" <<endl;
    cin>>barcode;
    file.open("book.dat",ios::in|ios::out);
    while(file.read((char*)&book1,sizeof(Book)) && found==0)
    {
        if(strcmp(retBarcode(),barcode)==0)
        {
            displayBook();
            editBook();
            long pos=-1*sizeof(book1);
            file.seekp(pos,ios::cur);
            file.write((char*)&book1,sizeof(Book));
            cout<<"Book updated"<<endl;
            found=1;
        }
    }

        file.close();
        if(found==0)
            cout<<"Error:Book Not Found. " <<endl;

}

void Book::deleteBook()
{
    fstream file, file2;
    char barcode[6];
    cout<<"Enter barcode of the Book you want to delete: " <<endl;
    cin>>barcode;
    file.open("book.dat",ios::in|ios::out);
    file2.open("Temp.dat",ios::out);
    file.seekg(0,ios::beg);
    while(file.read((char*)&book1,sizeof(Book)))
    {
        if(strcmp(retBarcode(),barcode)!=0)
        {
            file2.write((char*)&book1,sizeof(Book));
        }
    }

    file2.close();
        file.close();
        remove("book.dat");
        rename("Temp.dat","book.dat");
        cout<<"Book deleted." <<endl;

}

void Book::showallBooks()
{
    fstream file;
    file.open("book.dat",ios::in);
    if(!file)
    {
        cout<<"Error: File could not be opened. " <<endl;

            return;
        }

    while(file.read((char*)&book1,sizeof(Book)))
    {
        report();
        break;
    }

        file.close();

}

void Book::issueBook()
{
    fstream file,file1;
    char studentno[6],bookno[6];
    int found=0,sys=0;

    cout<<"Enter the Registration no. of the Student: "<<endl;
    cin>>studentno;
    file.open("student.dat",ios::in|ios::out);
        file1.open("book.dat",ios::in|ios::out);
        while(file.read((char*)&student2,sizeof(Student)) && found==0)
        {
        if(strcmp(student2.retregistrationNo(),studentno)==0)
        {
            found=1;
            cout<<"Enter barcode of the Book you want to issue: "<<endl;
            cin>>bookno;
            while(file1.read((char*)&book1,sizeof(Book))&& sys==0)
                {
                    if(strcmp(retBarcode(),bookno)==0)
                    {
                        displayBook();
                        sys=1;
                        student2.getstudentbookBar(retBarcode());
                        long pos=-1*sizeof(student2);
                        file.seekp(pos,ios::cur);
                        file.write((char*)&student2,sizeof(Student));
                        cout<<"Book issued";
                    }
                    }
                if(sys==0)
                        cout<<"Error: Book barcode does not exist. "<<endl;
            }

        }

        if(found==0)
        cout<<"Error: Student does not exist in the system. "<<endl;

    file.close();
    file1.close();
}

void Book::returnBook()
{
    fstream file, file1;
    char studentno[6];
    int found=0,sys=0;

    cout<<"Enter the Registration no. of the Student: "<<endl;
    cin>>studentno;
    file.open("student.dat",ios::in|ios::out);
    file1.open("book.dat",ios::in|ios::out);
    while(file.read((char*)&student2,sizeof(Student)) && found==0)
       {
        if(strcmp(student2.retregistrationNo(),studentno)==0)
        {
            found=1;
            while(file1.read((char*)&book1,sizeof(Book))&& sys==0)
            {
               if(strcmp(retBarcode(),student2.retstudentbookBar())==0)
            {
                displayBook();
                sys=1;
                long pos=-1*sizeof(student2);
                file.seekp(pos,ios::cur);
                file.write((char*)&student2,sizeof(Student));
                cout<<"Book returned. "<<endl;
            }
            }
          if(sys==0)
            cout<<"Error: Book barcode does not exist. "<<endl;
              }

      if(found==0)
    cout<<"Error: Student not found in the system. "<<endl;

  file.close();
  file1.close();
  }
};

DVD.h

#ifndef DVD_H_
#define DVD_H_

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>
#include "Media.h"

using namespace std;

class DVD : public Media{

    char director[20];

  public:
    void insertDVD();
    void searchDVD(char barcode[]);
    void updateDVD();
    void deleteDVD();
    void showallDVDs();
    void issueDVD();
    void returnDVD();

    void newDVD()
    {
        cout<<"Enter The DVD Name: " <<endl;
        fgets(name, sizeof(name), stdin);
        cout<<"Enter the Name of Director: " <<endl;
        fgets(director, sizeof(director), stdin);
        cout<<"Enter the DVD barcode: " <<endl;
        cin>>barcode;
        cout<<"DVD added to the library" <<endl;
    }

    void displayDVD()
    {
        cout<<"Enter the DVD Name: " <<endl;
        puts(name);
        cout<<"Enter the Name of Director: " <<endl;
        puts(director);
        cout<<"Enter the DVD barcode: " <<endl;
        cin>>barcode;
    }

    void editDVD()
    {
        cout<<"Enter the DVD Name you want to Update: " <<endl;
        cin>>barcode;
        cout<<"Enter New DVD Name: " <<endl;
        fgets(name, sizeof(name), stdin);
        cout<<"Enter New Name of Director: " <<endl;
        fgets(director, sizeof(director), stdin);
    }

    void report()
    {
        cout<<"_____________"<<" _________________"<<" ________________"<<endl;
        cout<<"|Name of DVD| "<<"|Director of DVD| "<<"|Barcode of DVD| " <<endl;
        cout<<" "<<name<<"-----------"<<director<<"---------------"<<barcode<<endl;
    }

};         //class ends here

#endif /* DVD_H_ */

DVD.cpp

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>
#include "DVD.h"
#include "Student.h"

using namespace std;

Student student3;
DVD dvd1;

void DVD::insertDVD()
{
    fstream file;
    file.open("dvd.bin",ios::out|ios::app);
    newDVD();
    file.write((char*)&dvd1,sizeof(DVD));
    file.close();
}

void DVD::searchDVD(char barcode[6])
{
    fstream file;
    int sys=0;
    file.open("dvd.bin",ios::in);
    while(file.read((char*)&dvd1,sizeof(DVD)))
    {
        if(strcmp(retBarcode(),barcode)==0)
        {
            displayDVD();
            sys=1;
        }
    }

    file.close();
    if(sys==0)
        cout<<"Error: No such DVD in System." <<endl;
}

void DVD::updateDVD()
{
    fstream file;
    char barcode[6];
    int found=0;
    cout<<"Enter barcode of the DVD you want to update: " <<endl;
    cin>>barcode;
    file.open("dvd.bin",ios::in|ios::out);
    while(file.read((char*)&dvd1,sizeof(DVD)) && found==0)
    {
        if(strcmp(retBarcode(),barcode)==0)
        {
            displayDVD();
            cout<<"Enter The New Details of DVD"<<endl;
            editDVD();
            long pos=-1*sizeof(dvd1);
            file.seekp(pos,ios::cur);
            file.write((char*)&dvd1,sizeof(DVD));
            cout<<"DVD Updated" <<endl;
            found=1;
        }
    }

        file.close();
        if(found==0)
            cout<<"Error: No such DVD in System. " <<endl;
}

void DVD::deleteDVD()
{
    fstream file,file2;
    char barcode[6];
    cout<<"Enter Barcode of the DVD you want to delete : " <<endl;
    cin>>barcode;
    file.open("dvd.bin",ios::in|ios::out);
    file2.open("Temp.bin",ios::out);
    file.seekg(0,ios::beg);
    while(file.read((char*)&dvd1,sizeof(DVD)))
    {
        if(strcmp(retBarcode(),barcode)!=0)
        {
            file2.write((char*)&dvd1,sizeof(DVD));
        }
    }

    file2.close();
        file.close();
        remove("dvd.bin");
        rename("Temp.bin","dvd.bin");
        cout<<"DVD deleted from the System. " <<endl;

}

void DVD::showallDVDs()
{
    fstream file;
    file.open("dvd.bin",ios::in);
    if(!file)
    {
        cout<<"Error: File could not be opened. " <<endl;

            return;
        }

    while(file.read((char*)&dvd1,sizeof(DVD)))
    {
        report();
        break;
    }
        file.close();
}

void DVD::issueDVD()
{
    fstream file,file1;
    char studentno[6],bookno[6];
    int found=0,sys=0;

    cout<<"Enter the Registration no. of the Student: "<<endl;
    cin>>studentno;
    file.open("student.bin",ios::in|ios::out);
    file1.open("book.bin",ios::in|ios::out);
    while(file.read((char*)&student3,sizeof(Student)) && found==0)
    {
        if(strcmp(student3.retregistrationNo(),studentno)==0)
        {
            found=1;
            cout<<"Enter barcode of the Book you want to issue: "<<endl;
            cin>>bookno;
            while(file1.read((char*)&dvd1,sizeof(DVD))&& sys==0)
            {
                if(strcmp(retBarcode(),bookno)==0)
                {
                    displayDVD();
                    sys=1;
                    student3.getstudentbookBar(retBarcode());
                    long pos=-1*sizeof(student3);
                    file.seekp(pos,ios::cur);
                    file.write((char*)&student3,sizeof(Student));
                    cout<<"Book issued";
                }
            }
            if(sys==0)
                    cout<<"Error: Book barcode does not exist. "<<endl;
        }

    }

    if(found==0)
        cout<<"Error: Student does not exist in the system. "<<endl;

    file.close();
    file1.close();
}

void DVD::returnDVD()
{
    fstream file,file1;
    long found=0,sys=0;
    char studentno[6];
    cout<<"Enter the Registration no. of Student:" <<endl;
    cin>>studentno;
    file.open("student.bin",ios::in|ios::out);
    file1.open("book.bin",ios::in|ios::out);
    while(file.read((char*)&student3,sizeof(Student)) && found==0)
    {
        if(strcmp(student3.retregistrationNo(),studentno)==0)
        {
            found=1;
            while(file1.read((char*)&dvd1,sizeof(DVD))&& sys==0)
            {
               if(strcmp(retBarcode(),student3.retstudentdvdBar())==0)
               {
                displayDVD();
                sys=1;
                long pos=-1*sizeof(student3);
                file.seekp(pos,ios::cur);
                file.write((char*)&student3,sizeof(Student));
                cout<<"Book returned" <<endl;
               }
            }
          if(sys==0)
            cout<<"Error: No DVD found with the entered barcode." <<endl;
       }
    }

      if(found==0)
    cout<<"Error: Student not found in the system. " <<endl;

  file.close();
  file1.close();
  }

Student.h

#ifndef STUDENT_H_
#define STUDENT_H_

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>

using namespace std;

class Student
{
    char registrationno[6];
    char name[20];
    char stbookbar[6];
    char stdvdbar[6];

public:
    void insertStudent();
    void searchStudent(char regno[]);
    void updateStudent();
    void deleteStudent();
    void showallStudents();

    char* retregistrationNo()
    {
        return registrationno;
    }

    char* retstudentbookBar()
    {
        return stbookbar;
    }

    char* retstudentdvdBar()
    {
        return stdvdbar;
    }

    void getstudentbookBar(char t[])
    {
        strcpy(stbookbar,t);
    }

    void getstudentdvdBar(char t[])
    {
        strcpy(stdvdbar,t);
    }

    void report()
    {
        cout<<"_________________"<<" _________________"<<endl;
        cout<<"|Name of Student| "<<"|Registration No|" <<endl;
        cout<<" "<<name<<"-----------"<<registrationno<<endl;
    }

    void newStudent()
    {
        cout<<"Enter the registration no. " <<endl;
        cin>>registrationno;
        cin.ignore();
        cout<<"Enter the name of the student " <<endl;
        fgets(name, sizeof(name), stdin);
        stbookbar[0]='/0';
        cout<<"Student added to system." <<endl;
    }

    void displayStudent()
    {
        cout<<"Enter the registration no. : " <<endl;
        cin>>registrationno;
        cin.ignore();
        cout<<"Enter the name of the student: " <<endl;
        puts(name);
    }

    void editStudent()
    {
        cout<<"registration no. : " <<endl;
        cin>>registrationno;
        cout<<"Modify Student Name : " <<endl;;
        fgets(name, sizeof(name), stdin);
    }

};         //class ends here


#endif /* STUDENT_H_ */

Student.cpp

#include <stdlib.h>
#include <iostream>
#include <fstream>
#include<iomanip>
#include "Student.h"

using namespace std;

Student student1;

void Student::insertStudent()
{
    fstream file;
    file.open("student.dat",ios::out|ios::app);
    newStudent();
    file.write((char*)&student1,sizeof(Student));
    file.close();
}

void Student::searchStudent(char regno[])
{
    fstream file;
    int flag=0;
    file.open("student.dat",ios::in);
    while(file.read((char*)&student1,sizeof(Student)))
    {
        if((strcmp(retregistrationNo(),regno)==0))
        {
            displayStudent();
            flag=1;
        }
    }

    file.close();
    if(flag==0)
            cout<<"Error: Student not found in the system. "<<endl;
}

void Student::updateStudent()
{
    fstream file;
    char regno[6];
    int found=0;
    cout<<"Enter Student Registration no. " <<endl;
    cin>>regno;
    file.open("student.dat",ios::in|ios::out);
    while(file.read((char*)&student1,sizeof(Student)) && found==0)
    {
        if(strcmp(retregistrationNo(),regno)==0)
        {
            displayStudent();
            cout<<"Enter student's new details"<<endl;
            editStudent();
            long pos=-1*sizeof(student1);
            file.seekp(pos,ios::cur);
            file.write((char*)&student1,sizeof(Student));
            cout<<"Student's details Updated"<<endl;
            found=1;
        }
    }

    file.close();
    if(found==0)
        cout<<"Error: Student not found in the system. "<<endl;

}

void Student::deleteStudent()
{
    fstream file, file2;
    char n[6];
    int flag=0;

        cout<<"Enter The registration no. of the Student You Want To Delete : "<<endl;
        cin>>n;
        file.open("student.dat",ios::in|ios::out);
        file2.open("Temp.dat",ios::out);
        file.seekg(0,ios::beg);
        while(file.read((char*)&student1,sizeof(Student)))
    {
        if(strcmp(retregistrationNo(),n)!=0)
                file2.write((char*)&student1,sizeof(Student));
        else
            flag=1;
    }

    file2.close();
        file.close();
        remove("student.dat");
        rename("Temp.dat","student.dat");
        if(flag==1)
            cout<<"Student deleted from the system. " <<endl;
        else
            cout<<"Error: Student not found in the system. " <<endl;
}

void Student::showallStudents()
{
        fstream file;
        file.open("student.dat",ios::in);
        if(!file)
        {
            cout<<"Error: File could not be opened. "<<endl;

            return;
        }

    while(file.read((char*)&student1,sizeof(Student)))
    {
        report();
        break;
    }

    file.close();

}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Your question is fine, but is there any aspect of the code that you prefer a review on ? (Any facet/aspect of the code can be reviewed but if you have some particular things to ask, you can add it) \$\endgroup\$ – Marc-Andre Apr 7 '15 at 19:31
  • \$\begingroup\$ @Marc-Andre there isn't a specific aspect that I want to be reviewed. I just want to review the whole code. \$\endgroup\$ – Donk Apr 7 '15 at 19:33
  • \$\begingroup\$ You might find it easier to have an enum of the type and an item class that has a property for the type. An item collection class, with sub collections of pointers for each type, to handle searching, adding, removing, and deleting, would round it out. Adding more types would only mean adding extra code in few appropriate place instead of designing a whole new class. \$\endgroup\$ – tinstaafl Apr 7 '15 at 20:15
  • \$\begingroup\$ You should probably read about SOLID principles of object-oriented programming. \$\endgroup\$ – Raz Megrelidze Apr 8 '15 at 16:42
  • \$\begingroup\$ method names like retstudentdvdBar are not acceptable. \$\endgroup\$ – Raz Megrelidze Apr 8 '15 at 17:14
3
\$\begingroup\$

Here are a number of observations that may help you improve your code.

Avoid multicharacter character constants

The code in Student.h includes this line:

stbookbar[0]='/0';

It's extremely likely that what you meant was actually '\0' which is a single character, rather than '/0' which is two. Another place this error was made is in Main.cpp in which a switch statement is written like this: cin.getline( name, 80); dvdOption = name[0];

            switch(dvdOption)
            {
                case '1':
                    dvd2.issueDVD();
                    processed = true;
                    break;
            // other cases...
                case '10':
                    default:
                    cout<<"Incorrect selection. Please select from the given options." <<endl;
                    processed = false;
                    break;
            }

The problem is that case '10': will never occur because that is two characters and not one.

Use consistent formatting

As the above snippet of code shows, the formatting is not at all consistent. Using consistent levels of indentation will help both you and others read and correctly understand the code.

Omit spurious semicolons

In quite a number of cases in the code, there are spurious semicolons. For example in Main.cpp the first loop looks like this:

bool processed = false;
while(!processed)
{
    // other code
};

That trailing semicolon should not be there. It's not going to bother the compiler because it's simply treated as an empty statement which is almost undoubtedly optimized away, but it's quite distracting to human readers of your code, so all such spurious semicolons should be removed.

Be careful with if

The code in Main.cpp currently includes these lines:

    if (mainSelect == '1')
    // User Menu

        bool processed = false;
        while(!processed)
        {
            /// code
        }

The indentation suggests that you intended for the if statement to enclose the while statement but it does not because you have no {} after the if statement. The effect, then, of this part of the code is as though you had written this:

    if (mainSelect == '1') {
        bool processed = false;
    }
    while(!processed)
    {
        /// code
    }

That means that the temporary variable processed created within the loop is initialized and discarded, and then the old value of processed is used for the while loop. It's highly unlikely that's what you intended. Beginning programmers are sometimes counseled to always use enclosing braces for control flow constructs such as if, for and while -- this is why!

Include all necessary headers

The header file Student.h uses strcpy a number of times, but does not include the matching header file <cstring>. You should always include all required headers. In this case, however, I would advocate using std::string objects instead of all of these char arrays. The code looks like C or very old style C++.

Think about the user of your code

Right now, when the user runs your code they see ... nothing! There is no prompt or any indication that anything is happening. Better user interface design would be to present a menu or at least tell the user something to indicate that the program is actually running. Perhaps that was code that was omitted and would normally go where the comment // User Menu is currently in the code. Either way, that brings us to the next point.

Don't Repeat Yourself (DRY)

The code for main is almost 200 lines long and is very repetive code. Instead of doing things that way with extremely long nested switch statements, a better, more object-oriented way to do that would be to create a Menu object and use it to both present the menu choices and get the responses. Your code would be much easier to follow and to maintain if you rewrite it that way.

Use better variable names

Names such as student4 and book2 are a bit confusing. Are there really two or more book variables? Is there some student3 I'm overlooking? In Student.cpp, there are variables named flag and file which are not at all descriptive. Try to use variable names which indicate not just the type of variable, but the significance of it. So for example file could be called studentData instead which tells what it means rather than how it's represented in the computer.

Intialize variables when declaring them where practical

The code for Student.cpp include this:

fstream file;
file.open("student.dat",ios::in);

These could and should instead be combined into a single line

fstream studentData("student.dat",ios::in);

Doing so means that the variable is actually usefully initialized immediately at construction. This is a desireable property of C++ class variables in particular because it removes the possibility of bugs by attempting to use a partially initialized object. It's such a famous and well-known idea that it's got a name, which is Resource Acquisition Is Initialization or RAII.

Don't abuse break

The Main.cpp file includes a while(1) as the main loop. Somewhere within that monstrously large loop, the program either breaks out of the loop or exits. Rather than forcing the reader of your program to go hunting for that (those?) statements, it's much better practice to put the loop exit condition at the top of the loop.

Don't abuse objects

The current Student.cpp contains this peculiar loop:

while(file.read((char*)&student1,sizeof(Student)))
{
    report();
    break;
}

Just those five lines contain a huge number of flaws. First, it's more break abuse as mentioned above. Second, using a low-level read to initialize an object is very poor practice. It's not portable (different compilers use different memory layouts for the same structures), and it treats an object variable as though it were just a slab of memory. Don't do that! Instead, write an extractor operator something like this:

friend std::istream& operator>>(std::istream& in, Student& s)
{
    return in >> s.registrationno >> s.name >> s.stbookbar >> s.stdvdbar;
}

Also, showallStudents() function is a member function of Student. This is bad design, because a Student should be a single Student. If you want a collection of Student objects, create a different class, or better yet, use an already existing one such as std::vector<Student> students; Worse yet, this member function relies on a file-scope variable student1 which is very fragile design. Instead, make the variable a local variable to that function.

Prefer std::string to char[]

The Student object includes this member variable:

char name[20];

But what happens if the name is larger than 20 characters long? A person named "James Silk Buckingham" would have to truncate his name to fit into the allocated 20 characters, which is a sad way to repay one of 19th century England's major public library advocates. Instead, you could use std::string objects which grow or shrink as needed.

Define and use object constructors

There is a newStudent() routine which combines I/O to ask for the student's data with initialization of a Student object. First, separater I/O from object manipulation to the extent practical (and it's very practical here). Second, define and use a constructor for the Student object that initializes all member data. (The current code fails to initialize stdvdbar.)

Use C++ rather than C header files

In a number of places in the code, old-style C headers such as <stdlib.h> are included. First, preferably, eliminate the need for them by using more modern constructs, such as elminating the need for <string.h> by using std::string instead and including <string>.

Don't expose internal object data to outside callers

The Media class includes this public member data and function:

char barcode[6];

char* retBarcode()
{
    return barcode;
}

First, the member data should really be private unless there's a compelling reason to do otherwise (and there is not in this case). Second, return a pointer to internal data is inviting disaster. Consider the following abuse:

Media med;
char *ptr = med.retBarcode();
ptr[3] = 'm';

We have just altered the middle of the barcode of the med object instance. Worse yet would be this:

ptr[50] = 'm';

Now it's pointing off the end of the barcode, but there's nothing prohibiting or even gently recommending against that kind of abuse. Better is to keep member data private and only allow access to copies of data.

Rethink your objects

If we go to a library, checking out a book or DVD is essentially the same process. I hand the librarian my library card and the bar code is read. You had the right idea by creating a base class Media but then created separate and incompatible issueBook and issueDVD member functions for the derived Book and DVD classes. Better design would be to have an issue member function in the base class and have derived classes inherit that member function.

Reduce coupling

The Book and DVD classes should not need to know anything about the Student class, but in this code, they do. This is not good design. What if a Professor wants to check out a book? Better would be to reduce the coupling so that each object can stand on its own.

Eliminate hardcoded file names

Hardcoded file names are not good design. Much better would be to allow them to be changed by the user, or at the very least, put them as a const std::string variable somewhere in the code.

There's much more, but that's all I have time and inclination for at the moment.

\$\endgroup\$
1
\$\begingroup\$
  1. Please avoid the usage of namespace as follows:

    using namespace std;
    

    instead call it whenever necessary like

    std::cout<<"Incorrect selection. Please select from the given options." <<"\n";
    
  2. To improve the performance use "\n" instead of std::endl.
  3. For your notice after exit(0) break statement is a piece of dead code:

    case '4':
        processed = true;
        exit(0);
        break;
    
  4. default is another case item, which should be aligned appropriately:

    case '5':
    default:
    
  5. There are a few occasions in which space alignment hurts readability.

  6. The if else statement in main() for mainSelect can be converted to an outer switch statement. Usually switch statements guaranteed to perform better with most of the compilers, compared to if else blocks.
  7. In class declaration of media, data members can be declared as private and public access can be enabled using get and set methods. It is better practice of encapsulation and always better than exposing the implementation details of a class to public.
  8. Media.h file does not need any of the given header files, so remove all and make it simple. Whenever you need a header file, please add into the file when it is required. I didn't see a requirement so far to include the header file #include <stdlib.h> so remove it if it is unnecessary. Please consider this suggestion in almost all files in your program.
  9. Instead of flooding class declaration with function definitions, please define the function outside the class declaration like void student::displayStudent() if you intend to make those functions implicitly inline by defining functions inside class declaration. While defining the outside class, use the keyword inline.

  10. After opening the file check the status of file descriptor and handle error before using it as follows; if your file descriptor is a null pointer you will get a segmentation fault.

    file2.open("Temp.dat",ios::out);
    file.seekg(0,ios::beg);
    while(file.read((char*)&student1,sizeof(Student)))
    
\$\endgroup\$
  • \$\begingroup\$ Regarding 2. please don't blindly give this advice as it requires the use of std::cout.sync_with_studio(false) to gain any benefit unless you are piping to file (and this seems to be an interactive scenario so this is not the case). Please see here for more information. \$\endgroup\$ – Emily L. Apr 8 '15 at 13:36
  • \$\begingroup\$ @EmilyL. you are mixing up with C standard with C++ features. This is my online reference for this argument parashift.com/c++-faq/endl-vs-slash-n.html. More over it is explained in Stroustroup's book "Programming in C++" \$\endgroup\$ – Steephen Apr 8 '15 at 16:18
  • \$\begingroup\$ I'm sorry Steephen but you are ignoring the fact that the C++ standard streams are by default synchronized with the C standard streams. Thus the C flushing policy (linked earlier) is governing the behaviour of the C++ standard streams until you call std::cout.sync_with_stdio(false). Until you do so, the streams are line buffered unless they can be determined to go to a non-interactive device and '\n' will also cause a flush. \$\endgroup\$ – Emily L. Apr 9 '15 at 8:15

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.