Book record system

Question

I'm still new to programming. I have made a book record system and I just wanted some coding review if there is any mistakes I should fix or any improvements that are needed. Here is the code I made

#include <iostream>
#define MAX 5
using namespace std;
struct record
{
int id;//stores id
float price;//store price
int qty;//stores quantity
record* next;//reference to the next node
};

 record* head;//create empty record
 record* tail;//the end of the record
 void push(record *& head, record *&tail, int id, float price, int qty)
{
if (head == NULL)
{
    record* r = new record;
    r->id = id;
    r->price = price;
    r->qty = qty;
    r->next = NULL;//end of the list
    head = r;
    tail = r;
}
else if (head != NULL && (MAX - 1))
{
    record* r = new record;
    r->id = id;
    r->price = price;
    r->qty = qty;
    r->next = head;
    head = r;
}
}

int  pop(record *&head, record *& tail)
{
if (head == NULL)
{
    cout << "No record in memory" << endl;
}
else if (head == tail)
{
    cout << "The record "<<"ID: " << head->id << "\nPrice: " << head->price << "\nQuantity: " << head->qty << "\n" << "was deleted" << endl; //CORRECTION HERE
}
else
{
    record* delptr = new record;
    delptr = head;
    head = head->next;
    cout << "The record " << delptr->id << ", " << delptr->price << ", " << delptr->qty << " was deleted" << endl; //CORRECTION HERE
    delete delptr;

}
return 0;
}


void display(record *&head)
{
record* temp = new record; //CORRECTION HERE
temp = head;
if (temp == NULL)
{
    cout << "No record in memory" << endl;

}
else
{


        cout << "Record : " << endl;
        while (temp != NULL)
        {
            cout <<"\nID: "<< temp->id << "\nPrice: " << temp->price << "\nQuantity: " << temp->qty <<"\n"<< endl;  //CORRECTION HERE
            temp = temp->next;
        }

}
}

int LinearSearch(record *&head) {
int key;
record *aux = head;
cout << "Enter ID to search";
cin >> key;
while (aux != NULL) {
    if (aux->id == key) 
    {
        cout << "I found it\n";
        cout << "\nID: " << aux->id << "\nPrice: " << aux->price << "\nQuantity: " << aux->qty << "\n" << endl; 
    }

    else{
        cout << "ID not found";
    }
    aux = aux->next;
}
return NULL;
}

int update(record *& head, record *&tail, int id, float price, int qty) {
int key;
record *aux = head;
cout << "Enter ID to search";
cin >> key;
while (aux != NULL) {
    if (aux->id == key)
    {
        cout << "I found it\n";
        cout << "\nID: " << aux->id << "\nPrice: " << aux->price << "\nQuantity: " << aux->qty << "\n" << endl;
        int id, qty;
        float price;
        cout << "Update your data please";
        cout << "Enter ID:";
        cin >> aux->id;   
        cout << "\nEnter Price: ";
        cin >> aux->price;
        cout << "\nEnter Quantity: ";
        cin >> aux->qty;
    }

    else {
        cout << "ID not found";
    }
    aux = aux->next;
}
return NULL;
}

char menu()
{
char choice;

cout << "\t::MENU::\n" << endl;
cout << "1. Add new record\n" << endl;
cout << "2. Delete record\n" << endl;
cout << "3. Show record\n" << endl;
cout << "4. Search record\n" << endl;
cout << "5. Update record\n" << endl;
cout << "6. Quit\n" << endl;
cout << "-----------------------\n" << endl;
cout << "\nEnter selection : " << endl;
cin >> choice;
return choice;
}

int main()
{
record* head;
record* tail;
head = NULL;
tail = NULL;
char choice;
do
{
    cout << "---------------------- - \n" << endl;
    choice = menu();
    switch (choice) {   //CORRECTION HERE
    case '1':
        int id, qty;
        float price;
        cout << "Enter ID:";
        cin >> id;   // Please correct yourself here, what is r here, r is not declared anywhere
        cout << "\nEnter Price: ";
        cin >> price;
        cout << "\nEnter Quantity: ";
        cin >> qty;
        push(head, tail, id, price, qty);
        break;
    case '2':
        pop(head, tail);
        break;
    case'3':
        display(head);
        break;
    case'4':
        LinearSearch(head);
    case '5':
        update(head, tail, id, price, qty);
    default:
        cout << "Quiting...\n";
        return 0;
    }

} while (choice != '6');

return 0;
}

Answer 1

Good Practices

1) using namespace std is considered a bad practice and can lead to errors when you start using code from several namespaces. You shouldn't use it.

2) You have indentation issues in your code, this makes reading more difficult. Also, try to chose and stick with how you use curly braces (starting at the end of the line or at the beginning of the next line).

Write C++ not C

1) In C++, it is more frequent to use const than #define (in your example you would use, const int MAX = 5) because define doesn't respect scopes and also because define values are replaced by the pre-processor and that can lead to weird magical numbers in your compilation errors and make your code harder to debug. Unless you're really struggling to make your application smaller in memory, you shouldn't use #define.

EDIT: As @Incomputable stated, you can also use constexpr instead ofconst. constexpr states that your variable can be evaluated at compile time, which can be useful depending on the situation.

2) You are not using any classes here. You said that you're still new to programming, so if you don't know what a class is, forget this point. If you do know what a class is, you should probably at least use one class to put your list code in it. The global variables in your code are the number one sign that you should have a class to put them in.

3) You have defined head and tail twice, one in the global scope and one in the main function. This is a mistake that you can easily avoid by following 2).

Naming

1) Usually a structure or a class name starts with a capital letter. In your code, it would be Record.

2) You have a lot of variables that have short names that do not help the reader to understand what they are used for. Naming your variables and functions correctly is a very important part of programming and you should take time to think about it. For example MAX doesn't say what is maxed by this value. This information should be contained in the name, something like MAX_LIST_SIZE.

3) Chose a naming convention and stick to it. For example LinearSearch and display are nammed differently for no obvious reasons.

Memory management

1) In pop(record *&head, record *& tail) you wrote:

record* delptr = new record;
delptr = head;

When you do this, the memory allocated at the first line is lost and you won't be able to delete it. You can simply write record* delptr; as delptr will point to the memory you need to deallocate. Currently you are cleaning the memory you used in your list, but you're also allocating some memory that you do not use and that you won't be able to delete later.

2) record* temp = new record; //CORRECTION HERE This is exactly the same as 1), your temp pointer do not need any memory has it will receive the address of head where the memory space you'll need is.

Single responsibility principle

This principle is usually applied to modules and classes but you can apply it to your functions at a smaller scale too. A simple way to do this is to only have your functions do what their signature say they do. For example, int LinearSearch(record *&head) should search for something starting from the head. It shouldn't use cin to ask the user what it needs to search because asking for a user input is a completely different job. You should add key as a function parameter and call cin before calling LinearSearch.