insert an elem into a link list

[neilcancer]

i wrote a function to insert an elem into a list, but it was
wrong,wrong,wrong! and i have no idea about why it was wrong. If anyone
know, leave your advice, thank you.

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

typedef struct lnode
{
int elem;
struct lnode *next;
//int length;
}listnode,*linklist;

int initial(linklist list);
int insert(linklist list,int i,int x);

int main()
{
int i,t;
linklist testlist;
testlist=NULL;
//p=&testlist;

t=initial(testlist);
printf("%d\n",t);

//for(i=0;i<100;i++)
// insert(testlist,i,i);
//t=get(testlist,1);

i=insert(testlist,1,1);
printf("%d\n%d",t,i);

return 1;
}

int initial(linklist list)
{
list=malloc(sizeof*list);
list->next=NULL;
list->elem=0;
//list->length=1;
return 1;
}

int insert(linklist list,int i,int x)
{
int j=0;
listnode *new_node=0;
listnode *plist;
plist=list;
if(i=1)
{
new_node=malloc(sizeof*new_node);
new_node->elem=x;
list->next=new_node;
return 3;
}
/* for(j=0;j<i-1;j++)
plist=plist->next;
*/
while(plist&&j<i)
{
plist=plist->next;
j++;
}
if(!plist||j>i-1) return 0;
new_node=malloc(sizeof*new_node);
new_node->next=plist->next;
new_node->elem=x;
plist->next=new_node;
return 1;
}

 

[Michael Mair]

i wrote a function to insert an elem into a list, but it was
wrong,wrong,wrong! and i have no idea about why it was wrong. If anyone
know, leave your advice, thank you.

Hint: Compile with the highest warning level, try to understand
all emitted warnings and get rid of them by removing the cause
(not by "shutting the compiler up"). In this case, this would
have helped you.
I just looked at your insert() function, nothing else.

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

typedef struct lnode
{
int elem;
struct lnode *next;
//int length;
}listnode,*linklist;

Note: It is almost always a bad idea to typedef a pointer
type. What do you think if you see
const linklist my_list;
This is not
const listnode *my_list;
but
listnode * const my_list;
i.e. if you want a const listnode * and use typedefs, then
you need a new typedef for const.
Apart from that, this hides the fact that you are dealing
with pointers which may lead to your making mistakes you
would not have for a plain listnode* declaration.

int insert(linklist list,int i,int x)

What is the meaning of the parameters?
Either document that or call the parameters appropriately.
For example:
int insertNodeAfter (listnode *predecessor,
int mode,
int value)
or
int insertNodeAtPosition (listnode *startNode,
int position,
int value)
tell much more about the function.

{
int j=0;
listnode *new_node=0;
listnode *plist;
plist=list;
if(i=1)

You almost certainly mean "i==1". A good compiler warns about
that if you let it. Alternatively, some people advocate a
"Constant == Variable" style -- in this case you get an error
if you write = instead of ==.

{
new_node=malloc(sizeof*new_node);

You forget to check whether malloc() was successful. Do not
omit that.

new_node->elem=x;

You forget to copy the successor of list to new_node:
new_node->next = list->next;
Do this always, even if you assume that list->next == NULL.
Or emit an error if list->next != NULL.

list->next=new_node;
return 3;

3? I thought it would be 42. Honestly, either use symbolic
constants or enumeration constants to give meaning to your
return values.

}
/* for(j=0;j<i-1;j++)
plist=plist->next;
*/
while(plist&&j<i)

Note: "j=0" is quite a way up. Consider repeating it or moving
it to immediately before the loop.

{
plist=plist->next;
j++;
}
if(!plist||j>i-1) return 0;

Reconsider the "j>i-1" part: As j>=i if the loop is not
terminated via plist==NULL, you effectively never reach the
code after this lines.

new_node=malloc(sizeof*new_node);
new_node->next=plist->next;
new_node->elem=x;

Question: Why do you treat "i==1" differently?
You have a gap position between list->next and
list->next->next where you cannot insert anything.

Use _one_ mechanism to insert an element.

You are doing two things with this function. Consider
writing functions for each of them, i.e.

listnode *goToNodeAtPosition (listnode *startNode,
int position);
int insertNodeAfter (listnode *predecessor,
listnode *node);
listnode *createNode (int value);
int createNodeAfterPosition (listnode *startNode,
int position,
int value)
{
listnode *pred, *new = NULL;
pred = goToNodeAtPosition (startNode, position);
if (pred) {
new = createNode(value);
if (new == NULL)
return RET_ERR_NODE_CREATION_FAILED;
else
return insertNodeAfter(pred, new);
}
return RET_ERR_NO_SUCH_POSITION;
}

It is probably easier to test these functions individually.
In addition, a list is not good for random access, so the
"insertNodeAfter()" function is the natural function to
introduce nodes whereas "createNodeAfterPosition()" is
probably unnecessary if the list is used correctly.
Note that "insert" is used for inserting an existing node.

plist->next=new_node;
return 1;
}

Cheers
Michael

 

[g.ankush1]

When u call the function initial , what u are doing is passing testlink
which is a pointer to a node. So when u call the function initial , the
formal argument gets the copy of the address and is allocated the
memory and thus gets a new memory allocation. But the change is not
reflected in the testlink. So the testlink still points to some garbage
memory. So either you need to return the pointer to the node from
initial or you need to use pointers to pointers.

 

[CBFalconer]

i wrote a function to insert an elem into a list, but it was
wrong,wrong,wrong! and i have no idea about why it was wrong. If
anyone know, leave your advice, thank you.

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

typedef struct lnode
{
int elem;
struct lnode *next;
//int length;
}listnode,*linklist;

At least name these things so one can tell what they are. Maybe
listnode and listnodeptr. A few blanks after commas etc. would help
readability.

int initial(linklist list);

What is the return value for?

int insert(linklist list,int i,int x);

What is the return value for, and what is the parameter i for?

.... snip code ...

Consider the following prototypes, returning NULL for failure:

linklist insert(linklist list, int x);

which will be called by something like:

if (tmp = insert(thelist, xvalue)) thelist = tmp;
else /* take corrective action */;

The only initialization needed is setting thelist to NULL. Then
insert could be coded as:

linklist insert(linklist list, int x)
{
linklist new;

if (new = malloc(sizeof *new)) {
new->elem = x;
new->next = list;
}
return new;
}

This inserts things at the head of a list, and is probably the
simplest.

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell.org/google/>
Also see <http://www.safalra.com/special/googlegroupsreply/>

 

[CBFalconer]

When u call the function initial , what u are doing is passing testlink
which is a pointer to a node. So when u call the function initial , the
formal argument gets the copy of the address and is allocated the
memory and thus gets a new memory allocation. But the change is not
reflected in the testlink. So the testlink still points to some garbage
memory. So either you need to return the pointer to the node from
initial or you need to use pointers to pointers.

Mr U did not ask anything, as might be evident if you had included
proper context. For means to do that, see below. Avoid silly
abbreviations in Usenet, which only serve to make things hard to
read and mark you as a beginner.

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell.org/google/>
Also see <http://www.safalra.com/special/googlegroupsreply/>