2

I have an assignment that requires me to sort a heap based C style array of names as they're being read rather than reading them all and then sorting. This involves a lot of shifting the contents of the array by one to allow new names to be inserted. I'm using the code below but it's extremely slow. Is there anything else I could be doing to optimize it without changing the type of storage?

//the data member
string *_storedNames = new string[4000];

//together boundary and index define the range of elements to the right by one
for(int k = p_boundary - 1;k > index;k--)
   _storedNames[k]=_storedNames[k - 1];

EDIT2: As suggested by Cartroo I'm attempting to use memmove with the dynamic data that uses malloc. Currently this shifts the data correctly but once again fails in the deallocation process. Am I missing something?

int numberOfStrings = 10, MAX_STRING_SIZE = 32;

char **array = (char **)malloc(numberOfStrings);

for(int i = 0; i < numberOfStrings; i++)
    array[i] = (char *)malloc(MAX_STRING_SIZE);

array[0] = "hello world",array[2] = "sample";   

    //the range of data to move
int index = 1, boundary = 4;
int sizeToMove = (boundary - index) * sizeof(MAX_STRING_SIZE);

memcpy(&array[index + 1], &array[index], sizeToMove);

free(array);
user2211776
  • 239
  • 1
  • 2
  • 11

4 Answers4

1

If you're after minimal changes to your approach, you could use the memmove() function, which is potentially faster than your own manual version. You can't use memcpy() as advised by one commenter as the areas of memory aren't permitted to overlap (the behaviour is undefined if they do).

There isn't a lot else you can do without changing the type of your storage or your algorithm. However, if you change to using a linked list then the operation becomes significantly more efficient, although you will be doing more memory allocation. If the allocation is really a problem (and unless you're on a limited embedded system it probably isn't) then pool allocators or similar approaches could help.

EDIT: Re-reading your question, I'm guessing that you're not actually using Heapsort, you just mean that your array was allocated on the heap (i.e. using malloc()) and you're doing a simple insertion sort. In which case, the information below isn't much use to you directly, although you should be aware that insertion sort is quite inefficient compared to a bulk insert followed by a better sorting algorithm (e.g. Quicksort which you can implement using the standard library qsort() function). If you only ever need the lowest (or highest) item instead of full sorted order then Heapsort is still useful reading.

If you're using a standard Heapsort then you shouldn't need this operation at all - items are appended at the end of the array, and then the "heapify" operation is used to swap them into the correct position in the heap. Each swap just requires a single temporary variable to swap two items - it doesn't require anything to be shuffled down as in your code snippet. It does require everything in the array to be the same size (either a fixed size in-place string or, more likely, a pointer), but your code already seems to assume that anyway (and using variable length strings in a standard char array would be a pretty strange thing to be doing).

Note that strictly speaking Heapsort operates on a binary tree. Since you're dealing with an array I assume you're using the implementation where a contiguous array is used where the children of node at index n are stored at indicies 2n and 2n+1 respectively. If this isn't the case, or you're not using a Heapsort at all, you should explain in more detail what you're trying to do to get a more helpful answer.

EDIT: The following is in response to you updated code above.

The main reason you see a problem during deallocation is if you trampled some memory - in other words, you're copying something outside the size of the area you allocated. This is a really bad thing to do as you overwrite the values that the system is using to track your allocations and cause all sorts of problems which usually result in your program crashing.

You seem to have some slight confusion as to the nature of memory allocation and deallocation, first of all. You allocate an array of char*, which on its own is fine. You then allocate arrays of char for each string, which is also fine. However, you then just call free() for the initial array - this isn't enough. There needs to be a call to free() to match each call to malloc(), so you need to free each string that you allocate and then free the initial array.

Secondly, you set sizeToMove to a multiple of sizeof(MAX_STRING_SIZE), which almost certainly isn't what you want. This is the size of the variable used to store the MAX_STRING_SIZE constant. Instead you want sizeof(char*). On some platforms these may be the same, in which case things will still work, but there's no guarantee of that. For example, I'd expect it to work on a 32-bit platform (where int and char* are the same size) but not on a 64-bit platform (where they're not).

Thirdly, you can't just assign a string constant (e.g. "hello world") to an allocated block - what you're doing here is replacing the pointer. You need to use something like strncpy() or memcpy() to copy the string into the allocated block. I suggest snprintf() for convenience because strncpy() has the problem that it doesn't guarantee to nul-terminate the result, but it's up to you.

Fourthly, you're still using memcpy() and not memmove() to shuffle items around.

Finally, I've just seen your comment that you have to use new and delete. There is no equivalent of realloc() for these, but that's OK if everything is known in advance. It looks like what you're trying to do is something like this:

bool addItem(const char *item, char *list[], size_t listSize, size_t listMaxSize)
{
    // Check if list is full.
    if (listSize >= listMaxSize) {
        return false;
    }
    // Insert item inside list.
    for (unsigned int i = 0; i < listSize; ++i) {
        if (strcmp(list[i], item) > 0) {
            memmove(list + i + 1, list + i, sizeof(char*) * (listSize - i));
            list[i] = item;
            return true;
        }
    }
    // Append item to list.
    list[listSize] = item;
    return true;
}

I haven't compiled and checked that, so watch out for off-by-one errors and the like, but hopefully you get the idea. This function should work whether you use malloc() and free() or new and delete, but it assumes that you've already copied the string item into an allocated buffer that you will keep around, because of course it just stores a pointer.

Remember that of course you need to update listSize yourself outside this function - this just inserts an item into the correct point in the array for you. If the function returns true then increment your copy of listSize by 1 - if it returns false then you didn't allocate enough memory so your item wasn't added.

Also note that in C and C++, for the array list the syntax &list[i] and list + i are totally equivalent - use the first one instead within the memmove() call if you find it easier to understand.

Cartroo
  • 4,233
  • 20
  • 22
  • The memmove() function seems to be what I'm looking for. The only problem is that I'm getting exceptions during deallocation of the array using delete[]. Is there a way to fix this? – user2211776 Apr 01 '13 at 11:45
  • You shouldn't be using `new` with memory that you're going to resize. You should use [`malloc()`](http://en.cppreference.com/w/c/memory/malloc) to allocate an initial block and then when you need more space you can use [`realloc()`](http://en.cppreference.com/w/c/memory/realloc). I suggest allocating more than you need and then doubling the allocation each time you run out of space as opposed to just what you need or you'll spend a lot of time in wasteful memory allocation. After the `realloc()` you can use `memmove()` to shuffle the data up. – Cartroo Apr 01 '13 at 14:22
  • Or, if you don't care to manage the memory yourself then you could use a `std::vector` and simply not worry about it - that will let you insert elements and handle everything for you. However, you stated that you didn't want a different structure, and using C arrays means handling the memory yourself. Note that `realloc()` leaves any contents unchanged, but the new memory is uninitialised, so you'll need to separately keep track of the amount of "valid" memory in the allocated block at all times. – Cartroo Apr 01 '13 at 14:24
  • The problem with using the malloc method is that I have coding standards which require me to use the the new and delete keywords for dynamic memory rather than malloc and free(). Is there any way to do it using those? – user2211776 Apr 01 '13 at 17:36
  • You can use `new` and `delete` as long as you know everything in advance, and it sounds like you do. See my updated answer above. – Cartroo Apr 01 '13 at 18:51
0

I think what you're looking for is heapsort: http://en.wikipedia.org/wiki/Heapsort#Pseudocode

An array is a common way to implement a binary search tree (i.e. a tree in which left children are smaller than the current node and right children are larger than the current node).

Heapsort sorts an array of a specified length. In your case, since the size of the array is going to increase "online", all you need to do is to call change the input size that you pass to heapsort (i.e. increase the number of elements being considered by 1).

maditya
  • 8,626
  • 2
  • 28
  • 28
0

Since your array is sorted and you can't use any other data structure your best bet is likely to perform a binary search, then to shift the array up one to free space at the position for insertion and then insert the new element at that position.

kkuryllo
  • 340
  • 3
  • 12
0

To minimise the cost of shifting the array, you could make it an array of pointers to string:

string **_storedNames = new string*[4000];

Now you can use memmove (although you might find now that an element-by-element copy is fast enough). But you will have to manage the allocation and deletion of the individual strings yourself, and this is somewhat error-prone.

Other posters who recommend using memmove on your original array don't seem to have noticed that each array element is a string (not a string* !). You can't use memmove or memcpy on a class like this.

TonyK
  • 16,761
  • 4
  • 37
  • 72
  • Could you give some simple code examples on how I'd go about placing a new string and accessing it using that method. I'm not too familar with pointers to pointers on the heap. – user2211776 Apr 01 '13 at 11:47