3

I'm trying to learn how to use C (from C#) and malloc has been one of the things giving me trouble when it comes to arrays. I have a pretty simple function to take an array sorted in non-descending { -1, 0, 1, 2, 3} ) order, and its size, square each value, and sort it in ascending order based on the squared values.

/// <param name="nums">integer array nums</param>
/// <param name="size">Size of array</param>
/// <returns>Sorted and squared array</returns>
int *sortedSquaredArray(int* nums, int size)
{
    //Starting from both ends of the array, square and do a semi-merge sort
    int *sortedArray = (int *)malloc(size * sizeof(int));

    int startIdx = 0;
    int endIdx = size - 1;
    int cnt = size - 1;
    int a;
    int b;
    int c;

    while (startIdx < endIdx)
    {
        a = nums[startIdx] * nums[startIdx];
        b = nums[endIdx] * nums[endIdx];

        if (a >= b)
        {
            sortedArray[cnt] = a;
            startIdx += 1;
        }
        else
        {
            sortedArray[cnt] = b;
            endIdx += 1;
        }
        cnt -= 1;
    }

    //final loop
    c = nums[startIdx] * nums[startIdx];
    sortedArray[0] = c;


    return sortedArray;

When I pass the array back and try to print it (I'm in the console on Visual Studio) only the final value is set correctly in the sortedArray, which makes me think I'm writing to totally the wrong memory addresses, but I'm not sure why. I'm also not really clear when, if you pass the pointer back to another function, to free up the used memory from malloc.

/// Run the sortedSquaredArray test
/// </summary>
void runSortedSquaredArrayTest()
{
    int nums1[] = { -4, -1, 0, 3, 10 };

    int *res = sortedSquaredArray(nums1, 5);
    printf("val1 %d, val2 %d, val3 %d, val4 %d, val5 %d", res[0], res[1], res[2], res[3], res[4]);
    
}

I feel like an idiot for taking something as neat as being able to actually manually allocate the memory and making such a mess of it :/

Jirara
  • 105
  • 8
  • Your program won't sort in ascending order in general. For example, the array `{1, 3, 5, 4, 2}` won't be sorted. – MikeCAT Jun 25 '21 at 23:32
  • 1
    @paulsm4 No. Casting results of `malloc()` family is [considered as a bad practice](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – MikeCAT Jun 25 '21 at 23:32
  • 2
    @paulsm4: `int *sortedArray = malloc( size * sizeof *sortedArray );` is more exactly correcter. – John Bode Jun 25 '21 at 23:34
  • 1
    @paulsm4: Less typing (in both senses of the word) is better. Explicit casts *are* evil - their presence usually indicates a problem. There are times when you can't avoid them, but *in general* they should be avoided wherever possible. While it's not true anymore, there was a time where explicitly casting the result of `malloc` could suppress a useful diagnostic if you forgot to include `stdlib.h`, leading to problems at runtime. It was *always* bad practice, but K&R didn't have `void` so you didn't have a choice. – John Bode Jun 25 '21 at 23:41
  • Remember to free(res), have you thought of using an inplace sorting? Then you avoid worries about returning anything :) – Sorenp Jun 26 '21 at 06:07
  • by *square it,* do you mean a passed in 4x4 array is returned as a 16x16 array? Please clarify – user3629249 Jun 26 '21 at 14:22
  • Yeah, that += instead of -= for the endIdx was definitely a typo :( Fixed it now. The cast mentioned by paulsm4 is the one I originally tried, but VS gives me an error that "a value of type void can't be used to initialize an entity of type int* ". The explicit cast fixed it, and choosing size of int or size of *sortedArray was something I wasn't sure about (super helpful to know that it should be the pointer, not the int type). I'm not sure how to avoid the explicit cast, or if this is something specific to VS. – Jirara Jun 26 '21 at 14:46
  • Also updated the question to be more clear what I meant by square. – Jirara Jun 26 '21 at 14:46
  • Unrelated but using dynamic allocation in a managed application (C#) requires the programmer to be **very** cautious... – Serge Ballesta Jun 26 '21 at 15:51

3 Answers3

0

The line

endIdx += 1;

looks wrong because it will lead to out-of-range access.

Try using

endIdx -= 1;

or

endIdx += -1;

instead.

MikeCAT
  • 73,922
  • 11
  • 45
  • 70
0

As MikeCAT mentioned, endIdx should be going down instead of up when you pull values off the end.

Your "semi-merge sort" is also a bit dubious. Consider the array:

{ 2, 3, 1 }

Your algorithm will start by comparing the start and the end. 2 > 1, so it will place 2 at the end. Only after this will it look at 3. This will end up as:

{ 1, 3, 2 }

I'm also not really clear when, if you pass the pointer back to another function, to free up the used memory from malloc.

As implemented, your function allocates memory and creates an obligation for the caller to free it when they are done using it. This is similar to how calling "open" creates an obligation to the caller to later call "close". To address this, your runSortedSquaredArrayTest function should end with free(res);. An alternative way to implement this is to have the caller provide the memory they want the output placed in. When called, this could look like:

void sortedSquaredArray(int length, int* input, int* output) {...}
void runSortedSquaredArrayTest()
{
    int input[] = { -4, -1, 0, 3, 10 };
    int output[] = { 0, 0, 0, 0, 0 };

    sortedSquaredArray(5, input, output);
    printf("val1 %d, val2 %d, val3 %d, val4 %d, val5 %d", output[0], output[1], output[2], output[3], output[4]);
    
}

With this approach the caller doesn't need to free anything.

  • The toy problem in this case explicitly provides a pre-sorted array as the input, so I didn't have to contend with an array like {2 3, 1} - it would always be { 1, 2, 3}. The trick for this question was actually in handling the negatives with a squared value. I agree that for a general case, this sort variation would totally fail. I really like the idea (and it seems like a more technically appropriate one) of passing in an output array pointer and populating it in the other function rather than using malloc. But my main issue was how to use malloc correctly in this scenario. – Jirara Jun 26 '21 at 14:50
0

the following proposed code:

  1. cleanly compiles
  2. performs the desired functionality
  3. only uses standard C header files

and now the proposed code:

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

//from: https://www.programmingsimplified.com/c/source-code/c-program-bubble-sort
void bubbleSort( int *array, size_t n )
{
    for ( size_t c = 0 ; c < n - 1; c++)
    {
        for (size_t d = 0 ; d < n - c - 1; d++)
        {
            if (array[d] > array[d+1]) /* For decreasing order use '<' instead of '>' */
            {
                int swap       = array[d];
                array[d]   = array[d+1];
                array[d+1] = swap;
            }
        }
    }
}


/// <param name="nums">integer array nums</param>
/// <param name="size">Size of array</param>
/// <returns>Sorted and squared array</returns>
int *sortedSquaredArray( int* nums, size_t size )
{
    int *sortedArray = malloc(size * sizeof(int));
    
    if( ! sortedArray )
    { 
        return NULL;
    }

    memcpy( sortedArray, nums, size*sizeof(int) );
    
    bubbleSort( sortedArray, size );
    
    for( size_t i = 0; i<size; i++ )
    {
        sortedArray[i] *= sortedArray[i];
    }

    return sortedArray;
}


/// Run the sortedSquaredArray test
/// </summary>
void runSortedSquaredArrayTest( void )
{
    int nums1[] = { -4, -1, 0, 3, 10 };

    int *res = sortedSquaredArray(nums1, sizeof(nums) / sizeof( int ));
    if( res )
    {
        printf("val1 %d, val2 %d, val3 %d, val4 %d, val5 %d", 
                res[0], res[1], res[2], res[3], res[4]);
        free( res );
    }
}


int main( void )
{
    runSortedSquaredArrayTest();
}

a typical run of the program results in:

val1 16, val2 1, val3 0, val4 9, val5 100
user3629249
  • 16,402
  • 1
  • 16
  • 17
  • VS still tells me that "a value of type void cannot be used to initialize an entity of type int*" when I try to use this code. I'm starting to think this is just an issue with VS as an IDE - I'll probably have to tell it to ignore cast errors from void. – Jirara Jun 26 '21 at 18:56
  • re you talking about the call to `malloc()`? If so, then I suspect that that your source code file extension is not `c` – user3629249 Jun 27 '21 at 05:09
  • Aah, ok. I thought since I picked a C-type project I would be coding in C, but it seems VS expected me to be using C++ (the extension on my files is actually .cpp). So, I have learned a lot, but ultimately, my question came down to an error. – Jirara Jun 28 '21 at 01:31