0

I am quite new to coding and got stuck with a problem. I tried to solve it myself and have been googling a lot, but I still do not have a solution to this. Maybe one of you can help?

This is my code:

int main(int argc, char **argv) {
    struct node {
        char *str;
        int count;
        struct node *next;
    };

    struct node head = { argv[1], 1, NULL };
    for (int i = 2; i < (argc); i++) {
        for (struct node *p = &head; (p != NULL); p = p->next) {
            printf("%s,%s\n", argv[i], p->str);
            if (strcmp(argv[i], p->str) == 0) {
                printf("case1\n");
                p->count++;
                break;
            }
            else if ((strcmp(argv[i], p->str) != 0) && p->next) {
                printf("case2\n");
                printf("Adresse, auf die p zeigt: %p", &p);
                continue;
            }
            else if ((strcmp(argv[i], p->str) != 0) && (!p->next)) {
                printf("case3\n");
                struct node *oldhead = &head;
                head.str = argv[i];
                head.count = 1;
                head.next = oldhead;
                break;
            }

        }
    }

    // Print how many times each string appears

    return 0;
}

The goal is to create a linked list that contains all the arguments I gave to the main() when calling the program. If there is a duplicate, the structure should count them. For example, if i call the program like ./a.out foo fool foo the result should be a list of length two, where the first element contains the string "foo" and count 2, and the second element contains the string "fool" and has a count of 1. The problem is the else if-statement within the inner for loop. This is the only part where the inner for loop should actually be used and assign p->next to p. Unfortunately that is not happening. The result is that the inner for loop starts over and over again and the pointer p points to the same address all the time (I used printf to figure that out).

Does any of you have an idea what could be the problem here? I tried everything I could and tried to find a solution online ...

Thanks a lot!!!

Improve this question
6
  • 1
    Where are you allocating your other nodes? Commented Oct 13, 2018 at 17:40
  • . o O ( how comes every 2nd questioner says "here is my whole code" and doesn't include #includes?? its a PITA! ) Commented Oct 13, 2018 at 17:43
  • Why use one of the most inefficient data structures? Why implement it yourself as there is std::list<>? Commented Oct 13, 2018 at 17:44
  • @Swordfish Because there is no std::list<> in C. ? Commented Oct 13, 2018 at 17:45
  • 1
    @WhozCraig I swear, I saw the C++-tag. Well, the first question still stands. Commented Oct 13, 2018 at 17:47

3 Answers 3

Reset to default
1

The issue is in this part of the code 

   else if ((strcmp(argv[i], p->str) != 0) && (!p->next)) {
        printf("case3\n");
        struct node *oldhead = &head;
        head.str = argv[i];
        head.count = 1;
        head.next = oldhead;
        break;
    }

You need to allocate a new struct and then put its address in the last struct entry.

       else if ((strcmp(argv[i], p->str) != 0) && (!p->next)) {
            printf("case3\n");
            struct node *oldhead = p;
            p = (struct node *) malloc(sizeof(node));
            if (p == NULL) { .... manage the error ... }
            oldhead->next = p;
            p->str = argv[i];
            p->count = 1;
            p->next = NULL;
            break;
        }

Now you're creating nodes and stringing them together. You were effectively updating the same node before.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

thanks so much for your help. Really understood it now.
0 
        struct node *oldhead = &head;
        head.str = argv[i];
        head.count = 1;
        head.next = oldhead;

That's not creating a new node. It's just creating a new reference to the same node, therefore causing an infinite loop when you try to read the linked list until the end. Therefore, your program only ever has one node. You need to actually allocate and create new ones.

Improve this answer

3 Comments

thanks for your fast reply! That means, the problem is not actually the second case but the third?
@user10500064 If this answer solved your problem, can you mark it as accepted by clicking its check mark?
As @JosephSible indicated, upvote answers that help, select the one that best answers your question. That helps those that help you.
0

The main problem here is

struct node *oldhead = &head;

which you should do malloc:

struct node *oldhead = (struct node*) malloc(sizeof(struct node));

so you really allocate a piece of memory for your new node. And because you have malloc, you should do free at the end of your program as well:

while(...) {
   free(deepest_node)
}

the way you do the loop above is to go from the farthest node in the linked list all the way back to the head.

The other issue is that, you should not append your new node to head:

head.next = oldhead;

but should be to p, which is the last node in your linked list:

p -> next = oldhead;
Improve this answer

6 Comments

thanks a lot! I did not understand that this part of the code was the problem.. So thanks for helping!!
by the way, depends on which complier you use, a standard C code should not define variables in the middle of the code but only at beginning. Such as struct node *p in the loop is not a good practice or may trigger complier complains (but perfectly fine in C++)
Thank you for the advise! From now on I will only define variables at the beginning.
Should I use "free" at the end of the main function or at the very end of the code? I was thinking about using this loop: for (struct node *f = &head; f; f = f->next) { free(f); }
No, don't. You free f and try to read f->next after that, which is bad. A matter of where should you free, I would prefer as soon as you don't need the memory any more. Which in your case, end of main function and end of the code should be the same thing.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.