S
Shao Miller
WANTED: To write an array implementation of Stack.
WHAT I GOT: It works fine.
I think program is still little too complex. Any ideas to improve ?
/* Array implementation of Stack - Aho, Hopcraft and Ullman, page 68-70
* section 2.3. We will add element at the bottom and we will grow the
* stack to top. top_index will keep track of index where element was
* added as latest. We will use top_index to push/pop elements.
*
* VERSION 0.0.
*
*/
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
enum {
SIZE_NAME = 10, SIZE_ARR = 3,
VAL_SUCC = 0, VAL_ERR = 1
};
If you use a source control system such as 'git' or even simply have
'diff'-style patches, it might be nice to put each of the enumeration
values on their own lines. Otherwise, a change in choice of 'SIZE_NAME'
happens in the same line as 'SIZE_ARR', but 'SIZE_ARR' is not affected
by the change (but a reader might look twice to be sure).
struct elm
{
int num;
char name[SIZE_NAME+1];
};
struct elm_stack
{
int top_index;
struct elm* arr[SIZE_ARR];
};
So it appears that your stack contains an array of pointers to elements,
rather than an array of the elements.
It appears to be a pretty short stack.
Just following along "out loud."
int push(struct elm_stack* , struct elm* );
void pop(struct elm_stack* );
Above, if 'push' and 'pop' are supposed to have a symmetrical sense
about them, you might wish to return the "popped" element with 'pop',
since you are offered a "pushed" element in 'push'. If a "popped"
element needn't ever be examined, I guess it doesn't matter.
struct elm_stack* stack_init(void);
void print_stack(struct elm_stack* );
struct elm* get_element(const char* c, int n);
Above, it appears that 'n' might be used as an index into the stack.
That suggests to me that the kind of stack you are thinking of is not
restricted to examination of only the top value. Ok.
int main(void)
{
struct elm_stack* s = stack_init();
'stack_init' must be responsible for allocating the stack or returning a
reference to a stack with static duration, by the looks of it.
struct elm* e0 = get_element("comp", 2);
struct elm* e1 = get_element("lang", 1);
struct elm* e2 = get_element("c", 0);
'get_element'? Hmm...
int ret_elem = push(s, e0);
if(VAL_ERR == ret_elem)
{
printf("IN: %s @%d: Memory Full\n", __FILE__, __LINE__);
free(e0);
}
ret_elem = push(s, e1);
if(VAL_ERR == ret_elem)
{
printf("IN: %s @%d: Memory Full\n", __FILE__, __LINE__);
free(e1);
}
ret_elem = push(s, e2);
if(VAL_ERR == ret_elem)
{
printf("IN: %s @%d: Memory Full\n", __FILE__, __LINE__);
free(e2);
}
print_stack(s);
printf("-----------------------------------------\n\n");
pop(s);
print_stack(s);
printf("-----------------------------------------\n\n");
pop(s);
print_stack(s);
printf("-----------------------------------------\n\n");
pop(s);
print_stack(s);
return EXIT_SUCCESS;
}
int push(struct elm_stack* s, struct elm* e)
{
int ret;
if(NULL == s || NULL == e)
{
printf("IN:%s @%d Invalid Args!\n", __FILE__, __LINE__);
ret = VAL_ERR;
}
else if(s->top_index == 0)
{
ret = VAL_ERR;
}
else
{
--(s->top_index);
/* printf("s->top_index = %d\n", s->top_index); */
s->arr[s->top_index] = e;
/* printf("%s = %d\n", e->name, e->num); */
ret = VAL_SUCC;
}
return ret;
}
It's interesting that a caller uses two steps to create an element on
the stack ('get_element', 'push'), but the 'pop' removes the element in
one step. You might reconsider this.
void pop(struct elm_stack* s)
{
if(NULL == s)
{
printf("IN:%s @%d: Invalid Args!\n", __FILE__, __LINE__);
}
else if(SIZE_ARR<= s->top_index)
{
printf("Nothing to pop()\n");
}
else
{
struct elm* e = s->arr[s->top_index];
free(e);
++(s->top_index);
}
}
Above, you do not set 's->arr[s->top_index]' to 'NULL' as you do in
'stack_init', below.
It seems that you are no longer interested in the element, so I can
understand why you do not return the element.
struct elm_stack* stack_init(void)
{
int idx;
struct elm_stack* p = malloc(1 * sizeof *p);
if(NULL == p)
{
printf("IN: %s @%d: Out of Memory!\n", __FILE__, __LINE__);
}
else
{
p->top_index = SIZE_ARR;
for(idx = 0; idx< SIZE_ARR; ++idx) p->arr[idx] = NULL;
}
Your choice for where you declare 'idx' above doesn't seem stylistically
consistent with where you declare it in 'print_stack', below.
If you 'return p;' within the 'if' block, you don't need the 'else'
block and can takes its content into the function's block. Why? If you
had further conditions for "success," you could end up with something like:
if (condition1) {
/* ... */
} else {
if (condition2) {
/* ... */
} else {
if (condition3) {
/* ... */
} else {
/* ... */
}
}
}
which is ever-indenting towards the right. Or you could have:
if (condition1) {
/* ... */
} else if (condition2) {
/* ... */
} else if (condition3) {
/* ... */
} else {
/* ... */
}
If all of the conditions are failure-checks and a reader recognizes
that, then they know that the last block depends on passing all tests.
Using 'return' in failure-checks is perhaps a more aggressive indication
that a check is a failure-check for the function, rather than making a
behavioural choice which doesn't affect the overall success of the
function. But that's just an opinion about style, too.
return p;
}
struct elm* get_element(const char* c, int n)
{
struct elm* p = malloc(1 * sizeof *p);
Why use '1 * sizeof *p' rather than 'sizeof *p', above?
if(NULL == p)
'if (!p)' is another option, but is a stylistic difference for people.
I don't think the behavioural equivalence will be changing any time soon.
{
printf("IN:%s @%d: Out of Memory!\n", __FILE__, __LINE__);
}
else if(SIZE_NAME< strlen(c))
{
printf("IN: %s @ %d: ", __FILE__, __LINE__);
printf("Name too big, Not adding element\n");
free(p);
p = NULL;
}
else
{
p->num = n;
The line above appears to suggest that the caller could potentially
specify duplicate indices.
strcpy(p->name,c);
}
return p;
}
Would 'make_element' or 'new_element' make more sense for the function
above? 'get_element' makes me think that an element already exists, but
you allocate one.
void print_stack(struct elm_stack* s)
{
if(NULL == s)
{
printf("IN:%s @%d: Invalid Args!\n", __FILE__, __LINE__);
}
else
{
int idx = SIZE_ARR - 1;
struct elm* e;
/*printf("idx %d, top_index = %d\n", idx, s->top_index); */
for(; (idx>= 0)&& (idx>= s->top_index); --idx)
{
Why not declare 'idx' without an initializer (as you do with 'e') and
then assign to it in the 'for' loop's "clause-1"?
If you used 'unsigned int' for 'idx', you could avoid potential signed
arithmetic overflow. Then 'idx >= 0' could also become 'idx'.
/*printf("idx %d, top_index = %d\n", idx, s->top_index);*/
e = s->arr[idx];
printf("%s = %d\n", e->name, e->num);
}
}
}
=================== OUTPUT ========================
[arnuld@dune C]$ gcc -ansi -pedantic -Wall -Wextra lifo-array.c
[arnuld@dune C]$ ./a.out
comp = 2
lang = 1
c = 0
It appears to work. I hope this brief review helps with "I think
program is still little too complex. Any ideas to improve ?"