HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Dynamic array stack and bag implementation

Submitted by: @import:stackexchange-codereview··
0
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

/*  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);

#endif


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

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 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 static

You 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: sizeDynArrDynArr_size

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:

  • 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 a DynArr and initializes it



  • DynArr_init – initializes an already allocated DynArr



  • DynArr_fini – deinitializes a DynArr



  • DynArr_del – deinitializes a DynArr and 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 values

C99 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_SELF
inline 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.