2

I'm trying to write a program which updates the smallest number of an array by itself iteratively until all the numbers in the array are the same (an algorithm for finding the lcm). My program seems to end up in an endless loop. My guess is that it is caused by are_same() returning always zero caused by something in get_min_index() and/or divisors[min_index] += divisors[min_index]. I'm wondering if this has to do with some C-language peculiarity I'm not aware of. Could some experienced C programmer help me out with this?

#include <stdio.h>

int are_same(int array[], int length) {
    for (int i = 1; i < length; i++) {
        if (array[0] != array[i]) {
            return 0;
        }
    }
    return 1;
}

int get_min_index(int array[], int length) {
    int min_value = array[0];
    int min_index = 0;
    for (int i = 1; i < length; i++) {
        if (array[i] < min_value) {
            min_value = array[i];
            min_index = i;
        }
    }
    return (min_index);
}

int main(void) {
    int divisors[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20 };
    int length = sizeof(divisors) / sizeof(divisors[0]);
    int min_index;

    do {
        min_index = get_min_index(divisors, length);
        divisors[min_index] += divisors[min_index];
    } while (are_same(divisors, length) == 0);

    printf("lcm=%d", divisors[0]);
    return(0);
}
jvkloc
  • 623
  • 1
  • 5
  • 13
  • `are_same()` and `get_min_index()` both look fine to me. Why do you think you should double the smallest element to get the LCM? – Barmar Jul 05 '23 at 15:35
  • Try using a smaller example like `{1, 2, 3}`, and print the contents of the array after each iteration of the `do` loop. You'll see that repeated doubling never gets you close to the LCM (which is 6). – Barmar Jul 05 '23 at 15:38
  • 1, 2, 3 => 2, 2, 3 => 4, 2, 3 => 4, 4, 3 => 4, 4, 6 => 8, 4, 6 => 8, 8, 6 => 8, 8, 12 – Barmar Jul 05 '23 at 15:39
  • 1
    Rather than doubling, you should add the original divisor. That means you need a working copy of `divisors[]`. Let's call it `working[]`, initialized to the same contents as `divisors[]`. On each iteration, do `min_index = get_min_index(working, length);` and update `working[min_index] += divisors[min_index];` and check `are_same(working, length)`. Also you should change the `do while` into a simple `while` to deal with the case of the divisors all being the same initially. – Ian Abbott Jul 05 '23 at 16:02
  • @Barmar I don't think that doubling will get the result. But I did write that. The idea I have is what Ian Abbot wrote. – jvkloc Jul 05 '23 at 17:46
  • @IanAbbott If you want to post an answer, I'll accept yours. That's the idea from the Wikipedia page but I wrote it badly. Tahnk you for pointing that out. – jvkloc Jul 05 '23 at 17:56

2 Answers2

1

Your are_same implementation is fine and does the job.

The algorithm, to find the smallest and to that add its corresponding divisor, then search for the smallest and do the same repeatedly, has a bug since you double the value instead of adding the divisor to it.

Even properly implemented, it will likely take a long time to finish since you search through the whole array in order to just update one value once. It would be faster if you instead could add the product of its divisor muliplied by x so that it becomes at least as large as the largest value in the array.

A slight modification would then be to instead

  • Search for the largest value in the array.
  • Increase all entries in array with a multiple of the corresponding divisor so that the value becomes at least as large as the largest value.
  • Repeat until all values are equal.

I don't care about the index of the largest in this example, so this simply returns the largest value:

int largest_value(int arr[], size_t length) {
    int l = arr[0];
    for(size_t i = 1; i < length; ++i) {
        if(arr[i] > l) l = arr[i];
    }
    return l;
}

A calculate_lcm function would need to copy the incoming array (to fix the bug in your current implementation) and implement the algorithm described above:

int calculate_lcm(const int array[], size_t length) {
    // copy array in to a "working array", wa:
    int* wa = malloc(length * sizeof *wa);
    if(!wa) exit(1);
    memcpy(wa, array, length * sizeof *wa);

    while(!are_same(wa, length)) {
        // find the largest value in wa:
        int large = largest_value(wa, length);

        for(size_t i = 0; i < length; ++i) {
            // make wa[i] at least as big as `large`
            wa[i] += ((large - wa[i] + array[i] - 1) / array[i]) * array[i];
        }
    }

    int lcm = wa[0];
    free(wa);
    return lcm;
}

Demo

You could also skip calling all_same and just check if the largest value has changed since the previous iteration. If it has not, all numbers are the same:

int calculate_lcm(const int array[], size_t length) {
    int* wa = malloc(length * sizeof *wa);
    if (!wa) exit(1);
    memcpy(wa, array, length * sizeof *wa);

    for(int largest = array[0];;) {
        int newlargest = largest_value(wa, length);
        if(newlargest == largest) break;
        largest = newlargest;

        for (size_t i = 0; i < length; ++i) {
            wa[i] += ((largest - wa[i] + array[i] - 1) / array[i]) * array[i];
        }
    }

    int lcm = wa[0];
    free(wa);
    return lcm;
}

Demo

Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
  • Is it really better to create and destroy an array every time in the function instead of having a single copy of it all the time like in my original code? I suppose it is, if the array is huge and takes up space? I'll accept this as an answer unless @Ian Abbot appears in a few days and writes his comment as one. He pointed out my logic error first. – jvkloc Jul 08 '23 at 08:21
  • @jvkloc _"every time"_ - It's copying it _once_ and yes, It is better because it works while your approach does not. If the object being copied wasn't `int` but instead a _huge_ object, then `malloc`ing an array of multipliers instead of the actual values would be preferable, but, it would still require the step of somehow allocating (using `malloc` or a VLA) memory while keeping the original array of divisors intact. – Ted Lyngmo Jul 08 '23 at 08:58
  • Oh, sorry. I wrote the updating a bit differently. array_update, are_same and largest_value are called in a while loop in main. – jvkloc Jul 08 '23 at 09:37
  • @jvkloc What is `array_update`? Also, there is no `largest_value` function in your question. That's part of my small implementation adjustment to make it finish in seconds instead of hours. – Ted Lyngmo Jul 08 '23 at 09:40
  • I wrote the version where I use you suggestion a bit differently from your example. I didn't really check your code and wrote it based on the idea. Also, the question in the first comment was about the creation/destruction of an array in every function call. That seemed a bit wasteful compared to having a single copy available all the time. – jvkloc Jul 08 '23 at 09:43
  • @jvkloc Aha, yes, good approach! Implementing the idea will for sure make it "make sense" instead of just copying someone elses implementation straight off. You're doing it right I must say. – Ted Lyngmo Jul 08 '23 at 09:46
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/254403/discussion-between-jvkloc-and-ted-lyngmo). – jvkloc Jul 08 '23 at 09:47
0

Hello, I do not think so there is something wrong to do with language or compiler, though I have enhanced version of your code. you can try and check it it works for you.

#include <stdio.h>

int are_same(int array[], int length) {
    for (int i = 1; i < length; i++) {
        if (array[0] != array[i]) {
            return 0;
        }
    }
    return 1;
}

int get_min_index(int array[], int length) {
    int min_value = array[0];
    int min_index = 0;
    for (int i = 1; i < length; i++) {
        if (array[i] < min_value) {
            min_value = array[i];
            min_index = i;
        }
    }
    return min_index;
}

int calculate_lcm(int array[], int length) {
    int lcm = array[0];
    for (int i = 1; i < length; i++) {
        int a = lcm;
        int b = array[i];
        // Calculate the greatest common divisor (GCD) using Euclid's algorithm
        while (b != 0) {
            int temp = b;
            b = a % b;
            a = temp;
        }
        // Calculate the least common multiple (LCM)
        lcm = (lcm * array[i]) / a;
    }
    return lcm;
}

int main(void) {
    int divisors[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20 };
    int length = sizeof(divisors) / sizeof(divisors[0]);

    int lcm = calculate_lcm(divisors, length);

    printf("LCM = %d\n", lcm);
    return 0;
}
  • Hi prachi! You code gives `18044195` which is wrong answer. – jvkloc Jul 05 '23 at 17:49
  • Correct answer is reached by using my corrected code, it is `232792560`. It is possible to get it with Euclidean algorithm, too. But you have something wrong in the code. – jvkloc Jul 05 '23 at 17:57