"Restaurant" HackerRank challenge in C

I don't write C all that often, so things to look for would be memory leaks and such. I also recognize that I'm not validating the user input as well as I could - I could use some feedback on how I might do better at that with this particular problem. I'm interested in hearing everything you find wrong with this code, however.


The problem, in so many words, is, given a list of dimensions for
rectangular bread slices, what's the fewest number of pieces I could
make to divide up each slice into perfect squares without wasting any
bread (0 cuts being 1 piece).


Input should look like:

2
2 2
6 9

where the first line provides the number of bread slices, and the
subsequent lines provide the length and width of each slice.


Output:

1
6

Original problem description

Note: I'm aware I could have used Lehmer's or the Euclidean methods for finding the GCD, but I wanted to implement the problem this way first just for the challenge of writing the brute force implementation. Other than alternative GCD algorithms, though, I would be interested in any opportunities found for performance improvement.

```
#include
#include
#include
#include
#include
#include

void free_2d_array(int** arr) {
int i = 0;
while (arr[i][0]) {
free(arr[i]);
i++;
}
free(arr[i]);

free(arr);
}

void flag_error(char msg[]) {
puts(msg);
exit(EXIT_FAILURE);
}

// Gets the number of slices for this test according to the first input value from stdin.
int read_num_slices() {
int num_slices = 0;

scanf("%i", &num_slices);

if (num_slices == 0) {
goto error;
}

return num_slices;

error:
flag_error("ERROR: Could not parse the number of entries from first input line.");
return 1;
}

// Gets a single line from stdin and attempts to parse it into a 2D int array representing the dimensions of a slice.
// Returns [0,0] on error.
int* read_slice_dimension() {
static int loaf_dimension[2] = {0};

scanf("%i %i", &loaf_dim

Profiling & Testing Your Approach

-
Valgrind test:

$ valgrind ./original
==43831== Memcheck, a memory error detector
==43831== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==43831== Using Valgrind-3.11.0.SVN and LibVEX; rerun with -h for copyright info
==43831== Command: ./original
==43831==
--43831-- UNKNOWN mach_msg unhandled MACH_SEND_TRAILER option
--43831-- UNKNOWN mach_msg unhandled MACH_SEND_TRAILER option (repeated 2 times)
--43831-- UNKNOWN mach_msg unhandled MACH_SEND_TRAILER option (repeated 4 times)
2
2 2
6 9
==43831== Conditional jump or move depends on uninitialised value(s)
==43831== at 0x1003FBC3F: _platform_memchr$VARIANT$Haswell (in /usr/lib/system/libsystem_platform.dylib)
==43831== by 0x1001EFBB6: __sfvwrite (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x1001FA005: __vfprintf (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x10021F9CE: __v2printf (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x10021FCA0: __xvprintf (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x1001F5B91: vfprintf_l (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x1001F39F7: printf (in /usr/lib/system/libsystem_c.dylib)
==43831== by 0x100000E27: main (main.c:132)
==43831==
1
6
==43831==
==43831== HEAP SUMMARY:
==43831== in use at exit: 43,127 bytes in 427 blocks
==43831== total heap usage: 512 allocs, 85 frees, 49,319 bytes allocated
==43831==
==43831== LEAK SUMMARY:
==43831== definitely lost: 16 bytes in 1 blocks
==43831== indirectly lost: 0 bytes in 0 blocks
==43831== possibly lost: 13,090 bytes in 117 blocks
==43831== still reachable: 30,021 bytes in 309 blocks
==43831== suppressed: 0 bytes in 0 blocks
==43831== Rerun with --leak-check=full to see details of leaked memory
==43831==
==43831== For counts of detected and suppressed errors, rerun with: -v
==43831== Use --track-origins=yes to see where uninitialised values come from
==43831== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0)


Now, Valgrind is not just a leak checker. It can give us a
lot of useful insights into your program and how it's working
internally. For instance, you see this little snippet that Valgrind
output after I gave your program some input? It's pointing us to
this line in your code:

printf("%i\n", find_min_number_of_slices(slices[i]));


We now know that we are possibly accessing the contents of something
that is unassigned on this line and that it could result in undefined
behavior. My guess to Valgrind's issue with this line has something
to do with slices[i], but I can't quite say for sure since I am
just starting out with my use of this tool in everyday programming.

-
Bash time test:

$ printf '2\n2 2\n6 9' | time ./original
1
6
1
1925460560
Command terminated abnormally.
0.16 real 0.00 user 0.00 sys


So your program runs pretty fast, but we seemed to have uncovered
some sort of strange behavior that causes a segmentation fault in
your program.

Reviewing Your Approach

-
You have no need for the stdarg.h heading that you include since
you do not accept a variable amount of arguments in any of your
functions.

-
I notice in some areas that you use while loops instead of for
loops, but you still have the counter variable in which you don't use
after the loop has concluded.

int i = 0;
while (arr[i][0]) {
free(arr[i]);
i++;
}
free(arr[i]);
free(arr);


Why was this not written as a for loop?

for (int i = 0; arr[i]; i++)
{
free(arr[i]);
}
free(arr);


-
Your flag_error() function declaration accepts a char array, but in
your free_2d_array() you accept a double pointer to an int. I
would change the flag_error declaration to accept a char pointer
instead. Also, since you don't modify this array in any way within the function, it should be declared as a const parameter.

-
The goto in your read_num_slices() function is useless. Always
try to avoid goto, since it leads to spaghetti code.
Rewritten function:

int read_num_slices()
{
int num_slices = 0;

scanf("%i", &num_slices);
if (!num_slices)
{
flag_error("ERROR: Could not parse the number of entries from first input line.");
return -1;
}

return num_slices;
}


-
You declare the loaf_dimension variable in your
read_slice_dimension() to be static, but do you really want it to
retain the values every time you run through the function? My guess
is not, so I would remove that identifier.

int loaf_dimension[2] = {};


-
In your get_slices() method, merge the declaration and assignment of
slices that are on two separate lines into an initialization
declaration of one line.

-
Again, there is no need for the goto in your get_slices()
function.

-
Use the pow() function in next_square() since you have
included the math.h header anyways and it should be a bit faster.

-
Declarer your counter variable whenever you can within the `for

StackExchange Code Review Q#73517, answer score: 11