Re: LIFO in C, need your suggestions

Sumit <onga@xxxxxxxxx> writes:

created a LIFO in C, need your suggestion what else more can i do on it,
and what should i change to make it best. need your suggestion specially
in function - stack_is_empty() , stack_remove() and stack_free() i have
doubt on those function.

I wrote one more extra function copy(), because for using only one
function "strcpy()" from string library unnecessary i have to include
whole library <string.h> , that is what i wrote copy() function.
is it good idea?

I don't think so. I can't see what advantage you think you gain by
doing the extra work (and thereby maybe introduce more bugs).

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

enum
{
SIZE_ARR = 20,
FAIL = -1,
NO = 0,
YES = 1
};

I'd leave 0 and 1 unnamed and use the results in Boolean tests without
== but I see that you want to return FAIL in some cases. I think this
complicates the use with little benefit. I want to write tests like
this:

while (!stack_is_empty(s)) { /* ... */ }

The empty test can return FAIL but that is not interesting to anyone.
It can only fail when s == NULL which I can test myself. If there are
more complex failures, you will need a more general mechanism anyway
so this simple YES/NO/FAIL return is of limited use.

struct sstack_node
{
char name[SIZE_ARR];
struct sstack_node * next;
};

struct sstack_list
{
struct sstack_node * top;
};
<snip>
void stack_remove(struct sstack_list * s )
{
if( s == NULL)
{
printf(" there is no stack list \n");
return;

Why, having decided to have a FAIL return why does this function not
return FAIL?

I assume all the printing going on is just for testing, yes? General-use
functions should not normally print anything.

}

if( s -> top != NULL )
{
printf(" stack is not empty yet, still making it empty \n");
pop(s);
}

Eh? Did you mean 'while' rather than 'if' here? You should use still
your test, stack_is_empty, even in your own code!

stack_free( s );
printf("stack has been removed successfully \n");
}

void stack_free(struct sstack_list * sfree)
{
if( sfree != NULL)
{
free( sfree );
sfree = NULL;

I would not assign to sfree here, but I accept that some people feel
they must.

return;
}
printf("address is already free \n");
return ;

}

struct sstack_node* pop(struct sstack_list * s)
{
struct sstack_node * temp = NULL;

if( s == NULL)
{
printf("stack is not initialized \n");
return NULL;
}

if( s -> top == NULL )
{
printf("Stack is empty \n");
return NULL;
}
temp = s -> top;
if(s -> top == NULL)
{
printf("s t a c k - e m p t y \n");
return NULL;
}

Duplicate test here.

s -> top = s -> top -> next ;
return temp;
}

void push(struct sstack_list *s , char *name )
{
struct sstack_node * newframe = NULL;
newframe = stack_frame(name); /* got a new node for insert into stack */
if( NULL == newframe ) return;
if( NULL == s -> top ) /* chekcing first frame into stack */
s -> top = newframe;
else
{
newframe -> next = s -> top;
s -> top = newframe;
}

You don't need to test for an empty stack.

newframe->next = s->top;
s->top = newframe;

works for both cases.

}

struct sstack_node * stack_frame( char * name)
{
struct sstack_node * newframe = NULL;
newframe = ( struct sstack_node * ) malloc (1 * sizeof *newframe);
if( NULL == newframe )
{
fprintf(stderr,"error: malloc fail \n");
return NULL;
}
copy( newframe -> name , name);

You've written your own copy but not taken the chance to make it
safer! copy could be told how large the target array is.

newframe -> next = NULL;
return newframe;
}

int copy(char to[], char from[])
{
int i=0;
while(( to[i] = from[i] ) != '\0')
++i;

to[i]='\0';

This line is pointless. The loop can only stop when to[i] is set to
zero.

return i;
}

struct sstack_list* stack_refresh(struct sstack_list* s)
{
if( s == NULL )
{
printf("stack is not initlialized \n ");
return NULL;
}
if( s -> top != NULL)
{
printf("stack is not empty cannot refresh stack until it is not be empty \n");
return s;
}
s -> top = NULL;
printf("stack has been refreshed successfully \n");
return s;
}

What is this function for? It seems to set s->top to NULL but only if
s->top already NULL.

int stack_is_empty(struct sstack_list* s)
{
if( NULL == s )
{
printf("stack was not initlialized \n");
return FAIL;
}
else if( NULL == s -> top )
{
printf("stack is empty \n");
return YES;
}
else
{
printf("stack is not empty \n");
return NO;
}
printf("%s:%d impossible condition \n", __FILE__, __LINE__);
return NO;
}

struct sstack_list * stack_init( void )
{
struct sstack_list * s = NULL;
s = (struct sstack_list *) malloc (1 * sizeof *s);
if( NULL == s )
{
fprintf(stderr,"error: malloc fail \n");
return s;
}
s -> top = NULL;
return s;
}

I find your code slightly 'wordy'. I'd write:

struct sstack_list *stack_init(void)
{
struct sstack_list *s = malloc(sizeof *s);
if (s)
s->top = NULL;
else fprintf(stderr, "error: malloc fail\n");
return s;
}

but that is a matter of taste.

--
Ben.
.

Relevant Pages