patterncMinor
Dynamic array stack and bag implementation
Viewed 0 times
stackarraybagdynamicandimplementation
Problem
I've have methods for a stack and bag using a dynamic array in C. As far as I can tell everything is implemented correctly, but I'm wondering what I can do to improve this code.
dynArray.h
dynamicArray.c
```
#include
#include
#include "dynArray.h"
struct DynArr
{
TYPE data; / pointer to the data array */
int size; / Number of elements in the array /
int capacity; / capacity ofthe array /
};
/ ***
Dynamic Array Functions
** */
void initDynArr(DynArr *v, int capacity)
{
assert(capacity > 0);
assert(v!= 0);
v->data = (TYPE ) malloc(sizeof(TYPE) capacity);
assert(v->data != 0);
v->size = 0;
v->capacity = capacity;
}
DynArr* newDynArr(int cap)
{
assert(cap > 0);
DynArr r = (DynArr )malloc(sizeof( DynArr));
assert(r != 0);
initDynArr(r,cap);
return r;
}
void freeDynArr(DynArr *v)
{
if(v->data != 0)
{
free
dynArray.h
/* dynArr.h : Dynamic Array implementation. */
#ifndef DYNAMIC_ARRAY_INCLUDED
#define DYNAMIC_ARRAY_INCLUDED 1
# ifndef TYPE
# define TYPE int
# define TYPE_SIZE sizeof(int)
# endif
# ifndef LT
# define LT(A, B) ((A) < (B))
# endif
# ifndef EQ
# define EQ(A, B) ((A) == (B))
# endif
typedef struct DynArr DynArr;
/* Dynamic Array Functions */
DynArr *createDynArr(int cap);
void deleteDynArr(DynArr *v);
DynArr* newDynArr(int cap);
int sizeDynArr(DynArr *v);
void addDynArr(DynArr *v, TYPE val);
TYPE getDynArr(DynArr *v, int pos);
void putDynArr(DynArr *v, int pos, TYPE val);
void swapDynArr(DynArr *v, int i, int j);
void removeAtDynArr(DynArr *v, int idx);
/* Stack interface. */
int isEmptyDynArr(DynArr *v);
void pushDynArr(DynArr *v, TYPE val);
TYPE topDynArr(DynArr *v);
void popDynArr(DynArr *v);
/* Bag Interface */
int containsDynArr(DynArr *v, TYPE val);
void removeDynArr(DynArr *v, TYPE val);
#endifdynamicArray.c
```
#include
#include
#include "dynArray.h"
struct DynArr
{
TYPE data; / pointer to the data array */
int size; / Number of elements in the array /
int capacity; / capacity ofthe array /
};
/ ***
Dynamic Array Functions
** */
void initDynArr(DynArr *v, int capacity)
{
assert(capacity > 0);
assert(v!= 0);
v->data = (TYPE ) malloc(sizeof(TYPE) capacity);
assert(v->data != 0);
v->size = 0;
v->capacity = capacity;
}
DynArr* newDynArr(int cap)
{
assert(cap > 0);
DynArr r = (DynArr )malloc(sizeof( DynArr));
assert(r != 0);
initDynArr(r,cap);
return r;
}
void freeDynArr(DynArr *v)
{
if(v->data != 0)
{
free
Solution
Avoid name-space pollution
You have carefully suffixed all your function names with the name of the type they operate on. This is good.
On the other hand, your header file spams macro definitions for
Declare helper functions
You have some helper functions that are only needed inside the
The function
Consider putting the type name first:
As discussed above, putting the type name into the function name is good because it avoids name clashes with functions that operate on other types that might also provide similar operations.
I prefer prefixing the type name, though. This makes the textual appearance of the function names more cohesive and supports auto-completion.
I find it more readable to put an underscore between type name and function name.
Avoid confusing names
You have the following functions:
Without looking at the code, it is hard to guess what the functions do. I recommend you seek for pairs of matching names. For example:
Since
You might be interested in my answer to this question on Stack Overflow about initializing (non) opaque types.
Consider
C99 introduced `
#define DYN_ARRAY_CONCAT_R(FIRST, SECOND) FIRST ## SE
You have carefully suffixed all your function names with the name of the type they operate on. This is good.
On the other hand, your header file spams macro definitions for
LT, EQ and TYPE into each file that #includes it. This is evil. TYPE is a far too generic name that might legitimately be used otherwise. LT and EQ are even worse. Besides, you're never using them. I suggest you get rid of all three macros as they are not needed. If you do need to #define macros in public header files, prefix them with your package name, for example, DYN_ARRAY_TYPE.Declare helper functions
staticYou have some helper functions that are only needed inside the
dynamicArray.c file. You have suffixed them with an underscore to indicate this. It would be better to (additionally) declare them static. This way, they will be truly private to your implementation file and cannot clash with other functions in other files. It might also make the code smaller and faster.The function
createDynArr is never defined. On the other hand, there is initDynArr which is not declared in the header. This is good, because the function is not needed by clients. You should delete the declaration of createDynArr from the header file and make initDynArr a static function.Consider putting the type name first:
sizeDynArr → DynArr_sizeAs discussed above, putting the type name into the function name is good because it avoids name clashes with functions that operate on other types that might also provide similar operations.
I prefer prefixing the type name, though. This makes the textual appearance of the function names more cohesive and supports auto-completion.
I find it more readable to put an underscore between type name and function name.
Avoid confusing names
You have the following functions:
newDynArr
createDynArr/initDynArr(see above)
freeDynArr
deleteDynArr
Without looking at the code, it is hard to guess what the functions do. I recommend you seek for pairs of matching names. For example:
DynArr_new– allocates memory for aDynArrand initializes it
DynArr_init– initializes an already allocatedDynArr
DynArr_fini– deinitializes aDynArr
DynArr_del– deinitializes aDynArrand deallocates it
Since
DynArr is an opaque type, the init and fini functions need not / should not be exposed by the header file.You might be interested in my answer to this question on Stack Overflow about initializing (non) opaque types.
Consider
bool instead of int for logical valuesC99 introduced `
which provides true, false and bool. Using these makes your intent clearer than using 1, 0 and int.
Consider size_t (or ptrdiff_t) instead of int
Some people say that when a value must not be negative, an unsigned integer type should be used. Others argue that unsigned arithmetic is a confusing source of bugs and it was a mistake to chose unsigned integer types for array sizes. Anyway, this choice is now baked so tightly into the language that it will never change. Having to switch between signed and unsigned types combines the worst of both worlds so you might as well accept it and use size_t for your array sizes and indices consistently. If you still want to use a signed integer, use ptrdiff_t as it is guaranteed to be large enough. int is only required to be at least 16 bit wide although it will be 32 bit on almost any modern platform.
size_t and ptrdiff_t are provided by the header.
Be const correct
If a function does not modify the object, it should take it by pointer-to-const. For example, DynArr_size certainly shouldn't alter the DynArr object, so declare it like this.
size_t DynArr_size(const DynArr * self);
// ^^^^^
What is a TYPE?
Your header file #defines the macro TYPE (which should really be named DYN_ARRAY_TYPE) to int. It would be handy if I could #define DYN_ARRAY_TYPE double before #includeing your header and instead get a dynamic array of doubles. Except, it doesn't work like this. While the code will compile and (unfortunately) probably also link happily, it will invoke tons of undefined behavior when executed. The problem is that your implementation file is still compiled for int.
If you don't want your users to #define the element type, just spell out int or use a typedef in your header file that cannot be changed from the outside. (But please pick a more specific name than type.)
If you want to be generic, it can be done even in C but it is ugly. First, you have to put all your code into the header file. That's not too bad but don't forget to declare all functions as inline.
Second, you have to change your names according to the type via some macro magic. For example, instead of
size_t DynArr_size(const DynArr * self);
you would write
``#define DYN_ARRAY_CONCAT_R(FIRST, SECOND) FIRST ## SE
Code Snippets
size_t DynArr_size(const DynArr * self);
// ^^^^^size_t DynArr_size(const DynArr * self);#define DYN_ARRAY_CONCAT_R(FIRST, SECOND) FIRST ## SECOND
#define DYN_ARRAY_CONCAT(FIRST, SECOND) DYN_ARRAY_CONCAT_R(FIRST, SECOND)
#define DYN_ARRAY_SELF DYN_ARRAY_CONCAT(DynArr_, DYN_ARRAY_TYPE)
inline size_t
DYN_ARRAY_CONCAT(DYN_ARRAY_SELF, _size)(const DYN_ARRAY_SELF * self);
#undef DYN_ARRAY_SELFinline size_t
DynArr_float_size(const DynArr_float *self);TYPE getDynArr(DynArr *v, int pos)
{
TYPE value;
if(v != NULL && v->size != 0 && v->size >= pos) {
value = v->data[pos];
}
return value;
}Context
StackExchange Code Review Q#125896, answer score: 4
Revisions (0)
No revisions yet.