1

I've been working on my school CS Project for a couple of weeks, and I coded a text based program to manage a supermarket/shop in C++. I ran into some trouble when I was trying to store the database and read it from file storage.

Complete source here

I am aware that TurboC++ is an extremely outdated compiler, but it's a part of my prescribed syllabus, so there's no way out of it. (Thankfully, the next batch out will be learning python)

My main concept was to use a self referencial structure as the node for a singly linked list handled by a class and its functions.

struct product  //Self referencial structure to store the product details
{
    unsigned int code;
    char name[100];
    char category[40];
    unsigned int quantity;
    double price;
    product *next;
};
class list  //Class to handle the linked list of products
{
    private:
        product *head, *tail;
    public:
        list()
        {
            head = NULL;    //By default the list will be empty
            tail = NULL;
        }
        void DelList(); 
        void AppProduct(product *, unsigned int);
        void AddProduct(unsigned int);  
        product* FindProduct(unsigned int); 
        double CalcTotal();
        void ListProducts();    
        void ModProduct(product *);
        void SaveToFile();
        void LoadFromFile();
        product* PointToNewProduct(product);
        product* GetProductFromFile(ifstream &);
        product* GetHead()
        {
            return head;
        }
        void Clear();
};

store and cart are globally declared objects of class list storing the separate linked list.

I chose the linked list format primarily for the program, and decided to add saving to file and loading from file only later when I found out file handling was a compulsory part of the project. This is the code I wrote for the same -

void list::SaveToFile()     //self explanatory
{
    ofstream fout("DATABASE.DAT", ios::trunc);
    product * cur = head;
    product temp;
    while( cur != NULL )
    {
        temp = *cur;
        fout.write( (char *)&temp, sizeof(product) );
        cur = cur->next;
    }
    fout.close();
}
product * list::PointToNewProduct(product temp)     //copy the temp product details into a new pointer and return the pointer
{
    product * cur = new product;
    cur->code = temp.code;
    strcpy(cur->name, temp.name);
    strcpy(cur->category, temp.category);
    cur->price = temp.price;
    cur->quantity = temp.quantity;
    cur->next = NULL;
    return cur;
}

product * list::GetProductFromFile(ifstream& fin)       //retrieve a single product from the given file
{
    if( fin.eof() )
        return NULL;
    product temp;
    fin.read( (char *)&temp, sizeof(product) );
    cout<<temp.name;
    return PointToNewProduct(temp);
}
void list::LoadFromFile()       //Function to load struct from file and rebuild the linked list (only returning one item right now)
This is the code I wrote for the same -     //Update: I thought I fixed it, but its up to two items now, still not all
{
    ifstream fin("DATABASE.DAT", ios::in);      //Initialize the stream
    head = GetProductFromFile(fin);     //initialize head pointer
    product * cur = head;       
    product * nextptr;
    do {
        nextptr = GetProductFromFile(fin);      //get the next product in file
        cur = cur->next = nextptr;      //link product with the previous product
        tail = cur;     //make the current pointer the last one
    } while( !fin.eof() );
    tail->next = NULL;
    fin.close();
}

Now my problem is, that while I am able to write the linked list items to file properly, trying to read it results in it retrieving only 2 nodes, regardless of the number of nodes I have written to file.

A debugging session with my teacher led me to believe that this was because I had not been deleting the older linked list when loading one from file, and this problem of unfreed memory carried forward when writing to file. A destructor I had written to free up memory was never called, since my objects were globally declared, leading me to write the DelList(); function and call it before reading from file. This unfortunately did not solve the problem.

void list::DelList()
        {
            product * cur = head, * temp;
            while(cur != NULL)
            {
                temp = cur;
                cur = cur->next;
                delete temp;
            }
            delete cur;
            delete head;
            delete tail;
            head = NULL;
            tail = NULL;
        }

This is the code I wrote for the same - I also added some test code as a switchcase choice where I simply read from file without linking, and it also does not display the desired output.

case 7:
                    clrscr();
                    ifstream fin("DATABASE.DAT", ios::in);  
                    product temp;
                    fin.seekg(0, ios::beg);
                    while( fin.read( (char *)&temp, sizeof(product) ) )
                    {
                        //fin.read( (char *)&temp, sizeof(product) );
                        cout<<temp.name<<'\t'<<temp.category<<'\n';
                    }
                    getch();
                    break;

What's the problem with my code, and how do I fix it?

buggy
  • 23
  • 6
  • Have a look at [`while(!feof(...))` being wrong](https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong). Even though a C question, the issue is the same with C++ streams, and you virtually do the same. – Aconcagua Dec 11 '19 at 09:00
  • Off-topic: you are needlessly copying data around: `temp = *cur; fout.write( (char *)&temp, ...)` – why not simply `fout.write((char*) cur, ...);`? Similarly, you could read: `product* p = new product; fin.read((char*) p, ...); p->next = nullptr;` – Aconcagua Dec 11 '19 at 09:05
  • The `delList` function *cannot* solve the problem: You assign a new head right from the start within your load function, so whatever was there before gets lost. What the function, if called, *does* solve is the memory leak you had before, though. – Aconcagua Dec 11 '19 at 09:09
  • 2
    when you restart the program, memory adresses will be different. You cannot save `product *next;` to the file and later load it to get a valid address. You need a different way to "link" your nodes in the file – 463035818_is_not_an_ai Dec 11 '19 at 09:09
  • @formerlyknownas_463035818 That's actually correct, but does not apply here: `PointToNewProduct` sets that pointer to NULL, and then it is overwritten anyway within the do-while loop... – Aconcagua Dec 11 '19 at 09:40
  • Still the pointer is saved for nothing. Two structs might avoid that, `product` without the pointer to next and `struct node { product p; node* next; };`. Then you could iterate over the nodes and `write(reinterpret_cast(&cur->p), sizeof(cur->p));` – Aconcagua Dec 11 '19 at 09:43
  • Couldn't find the issue by analysis, so debugged the code myself. However, I couldn't reproduce any error, your code (albeit quite cumbersome) seems to work fine. How do you create your products? Have you possibly forgotten to null-terminate the strings inside? If so, you might possibly read beyond array bounds (UB) and your programme simply crashes... – Aconcagua Dec 11 '19 at 09:57
  • What's the difference between `clear` and `delList`??? `delList` is not implemented correctly: As long as cur points to a node, that one will be deleted. After the loop, you first delete a null pointer via cur (which is legal, but unnecessary) and then the head and tail pointers. These haven't been set to nullptr so far, but still point to nodes that already have been deleted (double deletion -> undefined behaviour!). So just drop these three additional deletes and you'll be fine. – Aconcagua Dec 11 '19 at 10:01
  • 1
    Off-topic: You should get used to implementing the constructor's initialiser list (not to be confused with `std::initializer_list`): `list() : head(nullptr), tail(nullptr) { }`, especially if complex objects are involved, you prefer direct initalisation by value over default initialisation and assignment afterwards (the latter usually more costly). More important: Some types (references, non-default constructible ones, constant members) *only* can be initialised that way. – Aconcagua Dec 11 '19 at 10:06
  • Off-topic 2: `pointToNewProduct` and `getProductFromFile` functions do not look like they should be part of the public interface. Consider making them private. If you let the former accept a reference (`product &`), even better a const reference, as you don't modify (`product const&`), you spare one unnecessary copy. – Aconcagua Dec 11 '19 at 10:09
  • 1
    Maybe here the problem: `gets(temp->name);` – you shouldn't be using `gets`, there's no way to prevent buffer overflow (actually, that function has even been removed from both C and C++ in later standards for being unsafe)! Maybe you entered a too long product name? Then the terminating null character is written outside the buffer (actually undefined behaviour!) and gets lost. Then if there isn't (by pure accident!!!) any arbitrary null byte contained within your struct, you'll be writing beyond the allocated memory and most likely suffer a crash... – Aconcagua Dec 11 '19 at 10:20
  • @Aconcagua Thank you so much for your comments! I removed the three unnecessary ```delete``` statements, but that didn't help. I'll take a look at the C post on ```eof```, seems like the most likely problem to me. Also I do realize I have wasted some space here and there, I'm still a novice at coding, so thanks for pointing that out. The product name and categories are both well within the defined array sizes, so I don't think that's the problem, I don't understand how it could lead to arbitrary null bytes within my code? – buggy Dec 11 '19 at 11:11
  • 'Arbitrary null byte' – I meant that differently: You first read name, if too long, its null byte will get placed inside `category` array, but then possibly get overwritten again when reading the latter. If that one too long, its null byte might get overwritten when reading quantity and price or when setting the pointer. If none of these contains a null byte for whatever reason, you'll suffer from the problem described. Another problem that can arise is, as reading into local temporary, that you might corrupt the stack... – Aconcagua Dec 11 '19 at 11:19
  • Oh, I see what you mean then. If that were the case however, then why would it end up displaying the first two records without any problems? [Here are some screenshots](https://postimg.cc/gallery/38jedp28u/) to clarify. – buggy Dec 11 '19 at 11:56
  • That might have been the case only on third data set, and thus crash there. Well, if you still get output afterwards, though, or can go on running the programme, that indeed won't be the issue. However, it's pretty cumbersome having to input all the data manually every time. You can test your routines much easier if you hardcode a specific set of data right at the beginning inside main or alike. Unlucky you: You didn't separate IO and data storage (which usually is a good idea), so you'll have to modify your classes a bit... – Aconcagua Dec 11 '19 at 16:01
  • By the way, is there a specific requirement that drove you into creating your own linked list? There's `std::list` already. In most cases, though, `std::vector` is more efficient as you can operate on contiguous memory instead of having your data spread all around in RAM. – Aconcagua Dec 11 '19 at 16:04
  • 1
    Turbo C++ doesn't support namespaces or the C++ Standard Template Library, so ```std::list```, ```std::vector``` and even ```std::string``` can't be used. It's a pre-ANSI compiler which is very annoying, it's like halfway between C and C++ ( so, C+ ?) – buggy Dec 11 '19 at 16:44

1 Answers1

1

I managed to solve my problem with a workaround, though I still don't know why my original code didn't work, though seemingly the problem is with reading and writing pointers to and from a file.

What I've done to fix the problem is change product to be a class, inherited from the base class prod, with prod containing the same things that the structure used to with the exception of the pointer to the next. Then I wrote a function Delink() to convert each product in the linked list to a prod and write it to file. Then while reading the file, I convert each prod I read into a product and link it back to construct the linked list again. This seems to have fixed my problem.

buggy
  • 23
  • 6