0

This might be a dumb question but basically this program which uses pointer lists but stops execution after the first use of my showliste function which I use to I print out the list and I have no idea why. If I remove the showliste function then it runs the rest of the code just fine however I really have no idea why since I don't modify anything in that function and its only purpose is to print out the elements.

If somebody could help me out that would be very useful. Thank you in advance!

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define max 10

struct book{
    char name[50];
    float price;
    struct book *next;
};
typedef struct book BOOK;

BOOK * createlist(BOOK *);
void showliste(BOOK *);
BOOK * deleteElem(BOOK *);

int main()
{
    BOOK *pstart = NULL;
    pstart = createlist(pstart);
    printf("\nHere is the ordered list: \n");
    showliste(pstart); //stops excution after this for some reason
    pstart = deleteElem(pstart);
    printf("\nHere is the list with the element deleted: \n");
    showliste(pstart);
    return 0;
}

BOOK * createlist(BOOK *pdebut)
{
    int i, choice = 0;
    BOOK *pparcour = NULL, *pprecedent = NULL, *pnew = NULL;
    for(i = 0; i < max && choice == 0; i++)
    {
        pnew = (BOOK *)malloc(sizeof(BOOK));
        printf("Enter name: ");
        fflush(stdin);
        gets(pnew->name);
        printf("Enter Price: ");
        scanf("%f", &pnew->price);
        if(pdebut == NULL)
        {
            pdebut = pnew;
        }
        else{
            pparcour = pdebut;
            pprecedent = NULL;
            while(pparcour != NULL && pnew->price > pparcour->price)
            {
                pprecedent = pparcour;
                pparcour = pparcour->next;
            }
            if(pprecedent == NULL)
            {
                pnew->next = pparcour;
                pdebut = pnew;
            }
            else{
                pprecedent->next = pnew;
                pnew->next = pparcour;
            }
        }
        printf("Do you want to continue? \n");
        printf("0 - Yes                1 - NO\n");
        printf("Choice: ");
        scanf("%d", &choice);
    }
    return pdebut;
}

void showliste(BOOK *pdebut)
{
    while(pdebut != NULL)
    {
        printf("Name: %s\n", pdebut->name);
        printf("Price: %.3f\n\n", pdebut->price);
        pdebut = pdebut->next;
    }
}

BOOK * deleteElem(BOOK *pdebut)
{
    char cible[50];
    BOOK *pprecedent = NULL, *pparcour = NULL;
    printf("Enter the name of the book you want to delete: ");
    fflush(stdin);
    gets(cible);
    pparcour = pdebut;
    pprecedent = NULL;
    while(pparcour != NULL && strcmpi(cible, pparcour->name))
    {
        pprecedent = pparcour;
        pparcour = pparcour->next;
    }
    if(pparcour == NULL)
    {
        printf("\nEntered name is not in the list!!!!\n");
    }
    else{
        if(pprecedent == NULL)
        {
            pdebut = pdebut->next;
            free(pparcour);
        }
        else{
            pprecedent->next = pparcour->next;
            free(pparcour);
        }
    }
    return pdebut;
}
  • pdebut is the head of the list.
  • pparcour is a pointer which I use to go through my list without modifying it.
  • pprecedent is basically the element just before pparcour, mainly used to add a new book in the correct position in a ordered list (if the price of the new book is smaller than the price located in pparcour->price)
Abra
  • 19,142
  • 7
  • 29
  • 41
  • 4
    A tip. Don't use gets. It can cause undefined behaviour when you input something beyond the expected size and potentially buffer overflow. – Rinkesh P Jun 01 '22 at 03:22
  • 3
    Re: the `gets()` function, see "[Why is the gets function so dangerous that it should not be used?](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used)" – larsks Jun 01 '22 at 03:28
  • thanks, I will definitely keep that in mind, mainly used it here because I was being lazy. – LinksBurner Jun 01 '22 at 03:46
  • 1
    That's rather a lot of code. If you want others to look at it, consider creating a [mcve], in this case probably removing the user input part and just directly inserting dummy data to the list. Also, indicate where the program "stops execution" and how. Use debugger (quicker) or debug prints (more tedious) to find the line. – hyde Jun 01 '22 at 05:11
  • 3
    But, while you have that user input there: did you verify that you don't get scanf parse errors? If not, do so. – hyde Jun 01 '22 at 05:16
  • 3
    Providing a sample input file would also help us to more quickly assist in debugging. Note that the argument to `createlist` is essentially superfluous, and just acts as a local variable. Changing its value does not update the value held in `main`. Aside: I would also suggest separating the functions of creating / adding / removing / destroying nodes and the functions which deal with I/O. – Oka Jun 01 '22 at 05:17
  • 4
    C Standard defines `fflush(stdin);` as Undefined Behavior. Unless you are on a MS compiler that provides a non-standard extension, `fflush(stdin);` isn't doing what you think it is. You never set `pnew->next = NULL;` leaving the `next` pointer uninitialized. See also: [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/995714) – David C. Rankin Jun 01 '22 at 05:31
  • damn it really was that single line (not setting pdebut->next to null), thank you so much, that's to evreyone and I do apologize for not making my code more readable, I'm still new here so I'm still not the best at asking questions and translating my code but I will try to improve. Thank you again! – LinksBurner Jun 01 '22 at 20:59

1 Answers1

3

These lines

pparcour = pdebut;
/* ... */
pparcour = pparcour->next;

setup access to the uninitialized next member of the recently malloc'd structure, which contains an indeterminate pointer value. Attempting to to read a price member via this indeterminate pointer value

while(pparcour != NULL && pnew->price > pparcour->price)

will invoke Undefined Behaviour on subsequent iterations of the loop.

Use calloc, or manually set the newly allocated node's next member to NULL.

for(i = 0; i < max && choice == 0; i++)
{
    pnew = malloc(sizeof *pnew);
    pnew->next = NULL;
    /* ... */
Oka
  • 23,367
  • 6
  • 42
  • 53