Library Management System

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
#include
#include
#include
#include
#include
#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>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>barcode;
book2.searchBook(barcode);
break;
case '7':
book2.showallBooks();
break;
case '8':
break;
case '9':

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 `. 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 ob

StackExchange Code Review Q#86181, answer score: 3