0

I've tried and searched for this problem. I receive '3221225477' as a return value if I try to delete a string from the list.

I've already tried using different approaches such as changing 'return value[];' in the const char* del(ListNodePtr *strPtr, value[]) function to 'return *value;' It would be much appreciated if you could tell me what should I change in my code.

This is how I defined my structure. I don't know if this is really good.

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

        struct listnode
        {
            char data[100];
            listnode *nextPtr;
        };
            typedef struct listnode ListNode;
            typedef ListNode *ListNodePtr;

This is the insert function:

        void insert(ListNodePtr *strPtr, char value[])
        {
            ListNodePtr previousPtr, currentPtr, newPtr;

            newPtr=(listnode*)malloc(sizeof(listnode));
            strcpy(newPtr->data,value);
            newPtr->nextPtr=NULL;

            previousPtr=NULL;
            currentPtr=*strPtr;
            if(newPtr!=NULL)
            {
                       while(currentPtr!=NULL && 
                                       strcmp(currentPtr->data,value)<0)
                {
                    previousPtr=currentPtr;
                    currentPtr=currentPtr->nextPtr;
                }

                if(previousPtr==NULL)
                {
                    newPtr->nextPtr=*strPtr;
                    *strPtr=newPtr;
                }
                else
                {
                    previousPtr->nextPtr=newPtr;
                    newPtr->nextPtr=currentPtr;
                }
            }else printf("%s was not inserted. Insuffiecient memory!",value);

        }

This is the 'delete' function:

        const char *del(ListNodePtr *strPtr, char value[])
        {
            ListNodePtr previousPtr, currentPtr, tempPtr;

            if(strcmp(value, (*strPtr)->data)==0) /*if the 
                               first node shall be deleted*/
            {
            /*delete node*/
            tempPtr=*strPtr;
            *strPtr=(*strPtr)->nextPtr;
            free(tempPtr);
            return *value;
            }
            else
            {
                previousPtr=*strPtr;
                currentPtr=(*strPtr)->nextPtr;
                while(currentPtr!=NULL && strcmp(value, 
                                          currentPtr->data)!=0)
                {
                    previousPtr=currentPtr;
                    currentPtr=currentPtr->nextPtr;

                }

                if(currentPtr!=NULL)
                {
                    tempPtr=currentPtr;
                     previousPtr- >nextPtr=currentPtr->nextPtr;
                    free(tempPtr);
                    return *value;
                }
            }


                return '\0';//if the node is not found

        }

This is the main() function:

int main()
{
    ListNodePtr startPtr;
    startPtr=NULL;

    int optiune;
    char nume[100];

    instructions();
    printf("? ");
    scanf("%d",&optiune);

    while(optiune!=3)
    {
        switch(optiune)
        {
            case 1: 
                printf("Enter name:");
                fflush(stdin);
                gets(nume);
                insert(&startPtr, nume);
                printList(startPtr);
                break;

            case 2:
                fflush(stdin);
                printf("Search by name to delete from list:");
                gets(nume);
                if(!Empty(startPtr))
                {
                    if(del(&startPtr, nume))
                    {
                        printf("%s was deleted!\n");
                        printList(startPtr);
                    }else printf("%s was not found!\n",nume);

                }else
                    printf("List is empty!");
                break;

            case 3:
                break;
            default:
                printf("No such option!\n\n");
                instructions();
                break;
        }
        printf("\n? ");
        scanf("%d",&optiune);
    }
    printf("Execution stopped.\n" );
    return EXIT_SUCCESS;
Vlad Botis
  • 15
  • 4
  • 2
    `return *value;` is a type error. Are you just ignoring compiler warnings? – melpomene Dec 31 '18 at 19:32
  • @chux `return '\0';` is actually fine (it's a null pointer constant). – melpomene Dec 31 '18 at 19:33
  • 1
    `fflush(stdin);` has undefined behavior. `gets` is unsafe and should never be used (it was removed from standard C). – melpomene Dec 31 '18 at 19:48
  • 1
    `return '\0';` is fine as far as the compiler is concerned. It makes no sense to a human reading the code, and should be changed to `return NULL;`. Even better, the `del` function should return a `bool` that indicates success or failure, since that's all the return value is useful for. – user3386109 Dec 31 '18 at 19:57

1 Answers1

1

There are a multitude of little problems with the code in the question, but when those are cleaned up, the core algorithms seem to work correctly. The indentation is a bit curious in places; I'll ignore that from here on.

The critique can start rather quickly, unfortunately:

struct listnode
{
    char data[100];
    listnode *nextPtr;
};
typedef struct listnode ListNode;
typedef ListNode *ListNodePtr;

This is a C question, but if the code ever compiled, it was because it was compiled by a C++ compiler. The listnode *nextPtr; is not valid in C (but would be in C++). At that line, the name struct listnode is known, but the type listnode without the struct prefix is not defined in any of the standard headers (under normal circumstances, at any rate). You need struct listnode *nextPtr; to be valid C. Or you need to move the typedef struct listnode ListNode; before the structure definition, and then use ListNode *nextPtr; in the structure. You should probably not define or use ListNodePtr — see Is it a good idea to typedef pointers? for a discussion of why. You might clean up the code like this. Note that structure tags (like ListNode) are in the tags namespace (along with union tags and enum tags), but the typedef name ListNode is in the distinct 'ordinary identifiers' namespace — so there is no conflict between the uses of ListNode in this code:

typedef struct ListNode ListNode;
struct ListNode
{
    char data[100];
    ListNode *nextPtr;
};

In what follows, 'cleaning up the types' refers to using ListNode instead of ListNodePtr or any other variant spellings.

After cleaning up the types, the code in insert() is OK; the error message should be printed to stderr, and spell-checked, and should end with a newline.

The code in del() is probably not in the function delete() because delete is a keyword in a C++ compiler and hence can't be used as a function name. Don't use C++ compilers to compile C — you get misleading results.

The declared return type of the function is const char *, but the returned values are not pointers. It would be best to change the function to return either an int or even a bool to indicate whether it successfully deleted the specified name.

There was also a spacing blunder at:

previousPtr - > nextPtr = currentPtr->nextPtr;

There shouldn't be spaces around either the dot . or arrow -> operators, and with a space between the - and the > you don't even formally have an arrow operator.

However, the core algorithm in del() seems to work OK. There probably is room for improvement.

Your code referenced undefined (and indeed undeclared) functions like Empty(), printList(), instructions() — when you are creating an MCVE (Minimal, Complete, Verifiable Example), you should not have such omissions. The code below provides simple implementations for each of these.

The code in main() showed a variety of problems. One is code repetition by not writing useful functions to do particular pieces of work. I don't know what your plan for the instructions() function was, but I retained it and reduced it to a single printf() that prints a single prompt (without a newline at the end). It is itself used from a function get_option() that deals with a number of fiddly details.

You should NEVER use the gets() function; read about why it is far too dangerous to use gets() — ever!. Plus, mixing scanf() with functions like fgets() is a bit fraught because scanf() leaves the newline in the input buffer. I created the functions get_option() and get_string() to handle the issues, and to deal with invalid (non-numeric) input, and unexpected EOF, etc. They also flush standard output but not standard input. See Using fflush(stdin) for a moderately nuanced discussion — suffice to say fflush(stdin) is not portable, though it does have defined behaviour on Windows, but it does very little anywhere else other than invoke undefined behaviour. Generally, avoid it. If you must use it, be aware that you limit the portability of your code (and it is generally unnecessary to do that).

With the 'loop with input and test at the top', the code in the body of main() is slightly simpler. I resequenced the tests in the case 2: code to avoid nesting ifs; it is generally best to do something after testing, rather than starting a new test.

Note that the error condition reports the value that was entered (read). This is valuable to users — if you see something unexpected as the reported value, you may be able to deduce what went wrong better.

Putting it all together gives you code like this:

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

typedef struct ListNode ListNode;
struct ListNode
{
    char data[100];
    ListNode *nextPtr;
};

extern void insert(ListNode **strPtr, char value[]);
extern bool delete(ListNode **strPtr, char value[]);

void insert(ListNode **strPtr, char value[])
{
    ListNode *previousPtr, *currentPtr, *newPtr;

    newPtr = (ListNode *)malloc(sizeof(ListNode));
    strcpy(newPtr->data, value);
    newPtr->nextPtr = NULL;

    previousPtr = NULL;
    currentPtr = *strPtr;
    if (newPtr != NULL)
    {
        while (currentPtr != NULL &&
               strcmp(currentPtr->data, value) < 0)
        {
            previousPtr = currentPtr;
            currentPtr = currentPtr->nextPtr;
        }

        if (previousPtr == NULL)
        {
            newPtr->nextPtr = *strPtr;
            *strPtr = newPtr;
        }
        else
        {
            previousPtr->nextPtr = newPtr;
            newPtr->nextPtr = currentPtr;
        }
    }
    else
        fprintf(stderr, "%s was not inserted. Insufficient memory!\n", value);
}

bool delete(ListNode **strPtr, char value[])
{
    ListNode *previousPtr, *currentPtr, *tempPtr;

    if (strcmp(value, (*strPtr)->data) == 0)
    {
        tempPtr = *strPtr;
        *strPtr = (*strPtr)->nextPtr;
        free(tempPtr);
        return true;
    }
    else
    {
        previousPtr = *strPtr;
        currentPtr = (*strPtr)->nextPtr;
        while (currentPtr != NULL && strcmp(value, currentPtr->data) != 0)
        {
            previousPtr = currentPtr;
            currentPtr = currentPtr->nextPtr;
        }
        if (currentPtr != NULL)
        {
            tempPtr = currentPtr;
            previousPtr->nextPtr = currentPtr->nextPtr;
            free(tempPtr);
            return true;
        }
    }

    return false;
}

static bool Empty(ListNode *ptr)
{
    return(ptr == NULL);
}

static void get_string(size_t size, char buffer[size])
{
    if (fgets(buffer, size, stdin) == 0)
    {
        fprintf(stderr, "Unexpected EOF on standard input\n");
        exit(EXIT_FAILURE);
    }
    buffer[strcspn(buffer, "\n")] = '\0';
}

static void instructions(void)
{
    printf("1 to add, 2 to delete, 3 to exit: ");
}

static int get_option(void)
{
    int optiune;

    instructions();
    fflush(stdout);
    if (scanf("%d", &optiune) != 1)
    {
        fprintf(stderr, "Failed to read option number\n");
        exit(EXIT_FAILURE);
    }
    int c;
    while ((c = getchar()) != EOF && c != '\n')
        ;
    return optiune;
}

static void printList(ListNode *ptr)
{
    for (int i = 0; ptr != NULL; i++)
    {
        printf("%d: %s\n", i, ptr->data);
        ptr = ptr->nextPtr;
    }
}

int main(void)
{
    ListNode *startPtr;
    startPtr = NULL;
    int optiune;
    char nume[100];

    while ((optiune = get_option()) != 3)
    {
        switch (optiune)
        {
        case 1:
            printf("Enter name: ");
            fflush(stdout);
            get_string(sizeof(nume), nume);
            insert(&startPtr, nume);
            printList(startPtr);
            break;

        case 2:
            printf("Search by name to delete from list: ");
            fflush(stdout);
            get_string(sizeof(nume), nume);
            if (Empty(startPtr))
                printf("List is empty!\n");
            else if (delete(&startPtr, nume))
            {
                printf("%s was deleted!\n", nume);
                printList(startPtr);
            }
            else
                printf("%s was not found!\n", nume);
            break;

        default:
            fprintf(stderr, "No such option (%d)!\n\n", optiune);
            break;
        }
    }

    printf("Execution stopped.\n");
    return EXIT_SUCCESS;
}

(I make functions static unless there's a header to declare them in, and another source file that will use them. This can even help with optimization since the compiler can tell how often a static function is called and my inline the code.)

Here's a sample run of the code above:

1 to add, 2 to delete, 3 to exit: 1
Enter name: Patricia
0: Patricia
1 to add, 2 to delete, 3 to exit: 1
Enter name: Persephone
0: Patricia
1: Persephone
1 to add, 2 to delete, 3 to exit: 1
Enter name: Piglet
0: Patricia
1: Persephone
2: Piglet
1 to add, 2 to delete, 3 to exit: 1
Enter name: Pooh
0: Patricia
1: Persephone
2: Piglet
3: Pooh
1 to add, 2 to delete, 3 to exit: 1
Enter name: Puss In Boots
0: Patricia
1: Persephone
2: Piglet
3: Pooh
4: Puss In Boots
1 to add, 2 to delete, 3 to exit: 1
Enter name: Pygmalion
0: Patricia
1: Persephone
2: Piglet
3: Pooh
4: Puss In Boots
5: Pygmalion
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Pooh
Pooh was deleted!
0: Patricia
1: Persephone
2: Piglet
3: Puss In Boots
4: Pygmalion
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Pygmalion
Pygmalion was deleted!
0: Patricia
1: Persephone
2: Piglet
3: Puss In Boots
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Patricia
Patricia was deleted!
0: Persephone
1: Piglet
2: Puss In Boots
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Piglet
Piglet was deleted!
0: Persephone
1: Puss In Boots
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Puss In Boots
Puss In Boots was deleted!
0: Persephone
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Quantum Gold
Quantum Gold was not found!
1 to add, 2 to delete, 3 to exit: 2
Search by name to delete from list: Persephone
Persephone was deleted!
1 to add, 2 to delete, 3 to exit: 3
Execution stopped.
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • 1
    Jonathan Leffler, one of the best! Thank you, sir! I'm sorry I've not checked Stack Overflow to review answers because it seemed like I wouldn't get any of them. Thank you again for your implication, your work ethic and for everything you've contributed to on this site. – Vlad Botis Jan 20 '19 at 14:48