There is a lot wrong, undortunately. As WhozCraig said, there's a lot of other posts on this topic, so you should have probably searched a little more before posting. But since you have, let's walk through some of the issues together.
nodeptr i;
nodeptr q;
nodeptr p;
nodeptr *plist;
Here you are declaring a ton of global variables, most with bad names. What's i? What's p? What's q? Further down, you redeclare variables with the same names. Some with the same type, others with different type. This make it confusing to know which variable you're referencing.
Generally speaking, avoid global variables and choose descriptive names. In this case, you can just get rid of i, p and q.
Also, you never initialize plist to anything; you should get into the habit of initializing variables to some sane default value. In this case, NULL could be appropriate, but since you do not use the variable at all it can be deleted.
nodeptr getnode(void)
{
nodeptr p;
p = (nodeptr) malloc(sizeof(struct node));
return p;
}
This is good, however in C you should not cast the result of malloc to a particular type as this is considered bad form and can cause subtle and hard to detect bugs. Just assign the return from malloc directly.
Secondly, you never check to make sure that malloc succeeded. Granted, it's unlikely that it would fail in your simple program, but you should get in the habit of checking the return value of functions that can fail.
And you should probably initialize the allocated memory to some default value, because the memory returned to you by malloc is full of junk. In this case, something like this seems appropriate:
if(p) /* only if we allocated memory. */
memset(p, 0, sizeof(struct node));
There are times when you could skip this, but clearing the memory is a sane default practice.
void freenode(nodeptr p)
{
free(p);
}
This is also fine, but you should consider verifying that p is not NULL before you call free. Again, this comes down to robustness, and it's a good habit to get into.
int main()
{
int i;
nodeptr *k;
int a;
int *px;
int r;
nodeptr end;
nodeptr s;
nodeptr start;
Again, here we have a lot of unitialized variables, but at least some of the names are a bit better. But notice what happens:
You declare a variable called i of type int. But you've already declared a global variable called i that is of type nodeptr. So now, the variable in the local scope (the int) shadows (that is, it hides it) the global variable. So inside main the name i refers to the int. This just adds to the confusion when someone is reading your program.
p = getnode();
q = getnode();
OK... so, over here you allocate two new nodes and make p and q point to those nodes. So far so good.
q = start;
p = end;
Oops... now this is a problem. We now make p and q point to wherever start and end respectively point to.
And where do those point to? Who knows. Both start and end are unitialized, so they can point to anything. From this point on, your program exhibits undefined behavior: this means that anything can happen. Most likely, in this case, it will just crash.
Unfortunately from here on down, things become a lot more confusing. Instead of trying to explain everything, I'll just give some general commentary.
for (i = 0; i < 6; i++) {
printf("enter value");
scanf("%d", &r);
p = getnode();
p->info = r;
q->next = p;
q = q->next;
}
This loop is supposed to read 6 integers and put them in our linked list. This seems like a simple thing to do but there are issues.
First of all, you never check the return of scanf to know if the input operation succeeded. As I said before, you should always check the return value of functions that can fail and handle the failure accordingly. But in this case, let's ignore that rule.
A big problem is that q points to a random location in memory. So we're in undefined behavior land.
Another big problem is that there are two cases to consider: when the list is empty (i.e. the first time we're adding a number to the list when i == 0) and when the list is not empty (i.e. every other time). The behavior in those two cases differs. When i == 0 we can't just blindly set q->next, because even if q didn't point to a random location, there would, conceptually, be no q the way it's used here.
What we need here is some extra logic: if this is the first node that we are creating, set q to point to that node. Otherwise, set q->next to that node and then do q = q->next.
Please note, also, that you never set p->next anywhere, which would cause your list to not be NULL-terminated (something that you rely on here and in other loops). The memset fix in getnode corrects this problem, but generally you should make sure that if your code expects a particular behavior ("an unlinked node's next pointer points to NULL; lists are NULL-terminated") you should have code to ensure that behavior.
q = start;
Again, here, we reset q to point to start which is still uninitialized and points to garbage.
while ((q->next) != NULL) {
printf("n%d", (q->next)->info);
q = q->next;
}
This is a classic printing loop. Nothing wrong here, per se, although I think that stylistically, those parentheses around q->next are overkill and make reading the code a little more difficult than it has to be. My guideline would be to only add parentheses when they're necessary to override the default evaluation order of C or when the parentheses helps to visually explain to the reader how to group the expression in his head when mentally parsing the code.
scanf("%d", &a);
end = getnode();
end->info = a;
end->next = NULL;
This is fine, except for the error-checking issue with scanf, although you don't prompt the user to enter a number. But you correctly and explicitly make end->next point to NULL which is great.
for (q = start; q->next != NULL; q = q->next)
;
Again, the problem here is that q is set to start which, unfortunately, still points to garbage.
q->next = end;
q = start;
while ((q->next) != NULL) {
printf("n%d", (q->next)->info);
q = q->next;
}
This is the second time you've had to type this code to print the list. Generally, you should avoid code duplication. If you find that you need a particular code block in more than one place, it makes sense to split it out into a function and use the function. This makes understanding and maintaining the code easier.
for (q = start; q->next->next != NULL; q = q->next)
;
This loop is tricky to understand because of the q->next->next bit. Ask yourself "if I'm reading this, am I immediately sure that q->next can't ever be NULL?" If you aren't, then you really ought to rewrite this loop.
freenode(q->next);
q->next = NULL;
q = start;
Again, q is made to point to start which is unitialized. But hey, if we haven't crashed yet... ;)
while (q->next != NULL) {
printf("n%d", (q->next)->info);
q = q->next;
}
And again... this should really be a function.
return 0;
}
For a better implementation I refer you to one of the many other questions asked here (just search for "linked list delete". The implementation in this question by Khalid Waseem could also be of help, but it's not very documented, so you will have to carefully study and analyze the code to make sure that you understand it.
