Ok, a list of things.
First, as Lazaro pointed out, main should be type int, not void. A return 0; is then implied, but most people add it for clarity's sake.
Second, on a matter of style: your use of nil is good, but you might consider using NULL like everyone else does. You can usually get it from but I think it's in if you're really worried about being precise.
Third, another note of style. Instead of the *(ptr).member syntax, you can use ptr->member. Example:
firs->num = number; // (*first).num = number;
first->next = NULL; // (*first).next = nil; and following note 2
Fourth, more style. Your for loop condition is whacky. Since you wrote it, it's probably obvious to you that you're creating nodes 2-13. Most people reading this would probabaly do a double-take at first. To make 12 more nodes, most people would just do for(int i = 0; i < 12; i++) .
Fifth, yet more style. Putting a blank line between for or if constructs is also confusing. The common styles are same line or next line. And, if the enclosed block only has a single statement, you can get away without braces. With respect to that last, having a blank line looks really weird, to me at least.
Now we get into solving problems (I've just been writing these as I read the code :o)
Sixth: Now that we've hit the real code, time for a breakdown.
if (first == nil)
{
first = newnode;
}
else
{ /* everything in the else block */ }
If first was somehow nil/NULL, you skip the sorting. Even though first hasn't had a chance to change since you assigned it a value, so this won't happen. But setting it to the last one probably isn't the best thing to do. Just pointing out the logical flaw.
Seventh, looking at your //endwhile and //endif , you seem to have forgotten that C++ uses curly braces to indicate blocks. Put them in whenever you have a for/while/if/else till you've got a bit more experience under your belt, just to get in the habit.
Eighth, this code is hard to understand. Put some comments.
Ninth, looking at the logic in the else block:
if ((*newnode).num <=(*first).num)
{
(*newnode).next = first;
first = newnode;
}
else
...
So, if the last node (newnode) is less than the first, make it be the first. Makes sense. But now your list is a big circle. Ignoring the line first = newnode for a sec, we get this scenario: newnode points to first, which points to the second, which points to the third, which points... to the twelfth, which points to newnode, which points to first... ick.
Tenth is the problem with braces. I have no clue how your code is supposed to be. Just having my editor indent things automagically, I get this output:
if (first == nil)
{
first = newnode;
}
else
{
if ((*newnode).num <=(*first).num)
{
(*newnode).next = first;
first = newnode;
}
else
q=first;
p=(*q).next;
if (p==nil)
(*first).next=newnode;
else
while ((*p).num < (*newnode).num && ((*p).next!=nil))
q=p;
p=(*p).next;
//endwhile
//endif
if ((*p).num >= (*newnode).num)
{
(*q).next=newnode;
(*newnode).next = p;
}
else
(*p).next=newnode;
//endif
}//endif
//endif
q=first;
cout<< "The list contains \n";
while (q != nil)
{
cout<<" The numbers are: " << (*q).num << "\n";
q = (*q).next;
} //endwhile
I think you meant for it to be like this:
else
{
if ((*newnode).num <=(*first).num)
{
(*newnode).next = first;
first = newnode;
}
else
{
q=first;
p=(*q).next;
if (p==nil)
{
(*first).next=newnode;
}
else
{
while ((*p).num < (*newnode).num && ((*p).next!=nil))
{
q=p;
p=(*p).next;
} //endwhile
} //endif
if ((*p).num >= (*newnode).num)
{
(*q).next=newnode;
(*newnode).next = p;
}
else
{
(*p).next=newnode;
} //endif
} //endif
} //endif
q=first;
cout<< "The list contains \n";
while (q != nil)
{
cout<<" The numbers are: " << (*q).num << "\n";
q = (*q).next;
} //endwhile
I'll assume that my braces are correct from here on, based on your comments.
Eleventh on the list:
q=first;
p=(*q).next;
if (p==nil)
{
(*first).next=newnode;
}
Same as part 6.
Twelfth is the only part which has a chance to do work (comments added):
while ((*p).num < (*newnode).num && ((*p).next!=nil))
// while (p is less than the last node && p has a next)
{
q=p; // q is now p
p=(*p).next; // p is the next one
}
Since this is the only loop, you can't really sort a list elsewhere. And you don't do any reordering here, so it wont get sorted here either. Not much else to say, but this really doesn't do anything.
13th:
if ((*p).num >= (*newnode).num)
{
(*q).next=newnode;
(*newnode).next = p;
}
else
{
(*p).next=newnode;
} //endif
at this point there's two possibilities for the state of p: 1) p is nil (and it went through the code in 12) which can't happen without bad input (which would cause a crash earlier anyways), or 2) it went through the loop and either p->num >= newnode->num or p == newnode. The else in this quoted segment will never run. The if part of it will take q->next and change it from p to newnode. Then newnode will point to p. That's the extend of swappint the order of nodes in your code.
Then, finally, your output loop will die from things already mentioned (e.g. the linked list is messed up). I tried running it with ascending values (1 to 13) and it looped on 13. I tried descending order (13 to 1) and it printed a loop (as mentioned earlier).
