3
\$\begingroup\$

There is a sequence of integer values, and a separate integer.

I was required to write a method to get all numbers from the given number sequence which can be divided exactly by the given separate integer.

Example

number sequence = 6, 12, 34, 96, 72, 16
separate integer = 8
result = 96, 72, 16

The code

/*******************************************************************************
* Program
*******************************************************************************/

#include <stdio.h>

void get_devidable_int(int array_data[], int seperate_value, int *array_length);

void get_devidable_int(int array_data[], int seperate_value, int *array_length)
{
    int *array_data2 = array_data;
    int new_lenght = 0;
    int *ptr = NULL;
    for (int x = 0; x < *array_length; x++)
    {
        if (array_data[x] % seperate_value == 0)
        {
            ptr = &array_data[x];
            array_data[new_lenght] = array_data[x];
            new_lenght++;
            ptr++;
        }
    }

    *array_length = new_lenght;
}

int main()
{
    int value = 8;
    int dataset[6] = { 4, 16, 12, 64, 54, 33 };
    int length = sizeof(dataset) / sizeof(*dataset);

    printf("INPUT**********************************\n");
    for (int y = 0; y < 6; y++)
    {
        printf("data[%x]:%d\n", y, dataset[y]);
    }
    printf("seperate number: %d\n", value);

    get_devidable_int(dataset, value, &length);

    printf("OUTPUT*********************************\n");
    printf("New length *******:%d\n", length);
    for (int x = 0; x < length; x++)
    {
        printf("return data [%d] : %d\n", x, dataset[x]);
    }
    printf("END************************************\n");


    return 0;
}

Please help with reviewing to begin my C programming :)

\$\endgroup\$
1
  • \$\begingroup\$ Do you have to use C? The algorithm std::partition comes in the C++ standard library, ready to use. \$\endgroup\$
    – JDługosz
    Nov 4 '21 at 15:16
5
\$\begingroup\$

First, a minor point: various misspelt words - correct versions are separate, length, divisible. Although this is a minor point, it can be important in larger programs, as incorrect and inconsistent spellings make searching within the code more difficult.


void get_devidable_int(int array_data[], int seperate_value, int *array_length);

void get_devidable_int(int array_data[], int seperate_value, int *array_length)
{

Although it's not wrong to provide a separate declaration, it's not necessary here, as the definition also acts as a declaration.


While int mostly works for short arrays, the correct type for an array size is size_t


int *array_data2 = array_data;

This variable seems unused.

int *ptr = NULL;

We only ever write to this variable, so we can remove that one, too.


Modified function

Applying these observations, we get:

void filter_multiples(int array_data[], int factor,
                      size_t *array_length)
{
    size_t new_length = 0;
    for (size_t x = 0;  x < *array_length;  ++x)
    {
        if (array_data[x] % factor == 0)
        {
            array_data[new_length] = array_data[x];
            ++new_length;
        }
    }

    *array_length = new_length;
}

Minor improvements to main():

  • Don't declare functions taking _unspecified_arguments:

      int main(void)
    
  • I think we should by using length rather than 6 here:

     for (int y = 0; y < 6; y++)
    
  • main() is a special function: we're allowed to omit the final return 0; and the compiler will insert that for us.

  • The printing loop is written twice - we could make a function to save repeating that.

Here's how I would write the array-printing function:

static void print_array(const char *title,
                        const int *array, size_t length)
{
    for (size_t i = 0;  i < length;  ++i) {
        printf("%s[%zu]: %d\n", title, i, array[i]);
    }
}

Then in main(), we can replace the loops, like this:

    print_array("data", dataset, length);
    print_array("return data", dataset, length);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ @chux Fixed that - thanks. \$\endgroup\$ Nov 1 '21 at 7:16
2
\$\begingroup\$

Comments

/*******************************************************************************
* Program
*******************************************************************************/

Ask yourself, why is this comment necessary?

On the other hand, look at just the (superfluous) declaration of

void get_devidable_int(int array_data[], int seperate_value, int *array_length);

What is missing here is info what this does and how to correctly use this function. Make it clear what the parameters mean and how they are used. Write a few sentences as comment to answer these questions. Make sure you write complete sentences! This makes sure the info is better and not just fragments of words that only help you and only at this moment to understand what this does. More an some of these aspects below.

Parameter int seperate_value

The name for this is "divisor", which tells everyone what this value is used for. It also suggests that it must not be zero.

Parameter int array_data[]

This is actually the same as int* array_data, which makes it clear that this is a pointer to non-const data which is modified. This isn't immediately obvious when you call a function that's named "get_...". Just for brevity, I'd also call this numbers and the according size just length. In general, "data" and similar things are rarely useful in a name.

Magic Numbers

int dataset[6] = { 4, 16, 12, 64, 54, 33 };
int length = sizeof(dataset) / sizeof(*dataset);

If you add an element, you will have to adjust the 6 as well. Just drop that value so that the compiler determines the size from the number of elements in the initializer list. If you want a fixed size instead, declare the size as a constant up front and then use that to declare the array.

Sizeof Operator

There are two forms how sizeof is used:

  • Applied to a type like sizeof (type). The type (e.g. int) is put in parentheses here.
  • Applied to an object like sizeof object. No need for parentheses here.

I'd use (sizeof array) / (sizeof *array) to compute the size of an array.

\$\endgroup\$
1
\$\begingroup\$

In addition to @Toby Speight good answer:

Parameter order

Leading with the array size allows for better static code analysis and is self documenting.

// void get_devidable_int(int array_data[], int seperate_value,
//     int *array_length);
void get_dividable_int(int *array_length, int array_data[*array_length],
    int separate_value);

Error checks

I'd use the return value to indicate parameter problems:

// Return error flag
int get_dividable_int(int *array_length, int array_data[*array_length],
    int separate_value) {
  if (array_length == NULL) return 1;
  if (*array_length < 0) return 1;  // Not needed with size_t *array_length
  if (*array_length > 0 && array_data == NULL) return 1;
  if (separate_value == 0) return 1;
  ...
  return 0;
}

Pathologically case: separate_value == -1

INT_MIN % -1 is UB, yet mathematically INT_MIN mod -1 is 0. A simple fix prevents that issue:

  if (separate_value == -1) separate_value = 1;
  // or 
  if (separate_value == -1) return 0; // As the array will not change.

I would not code separate_value == abs(separate_value) as that is UB with separate_value == INT_MIN.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.