-2

I edited the code. But now it's showing runtime error. Can anyone tell why ? This is a program to sum 2 arrays and display output in third array. I also wanted to know if this code could be optimized ?

void sumOfTwoArrays(int arr[], int size1, int brr[], int size2, int crr[])
{
    int k;
    if(size1>size2){
        k = size1;
    }
    else
        k = size2;

    int c = k;
    int r = 0;
    int i = size1-1;
    int j = size2-1;
    for(;i>=0&&j>=0;i--,j--){
        int n = arr[i] + brr[j] + r;
        if(n<=9){
            crr[c] = n;
        }
        else
        {
          int r = n/10;
          n = n%10;
          crr[c] = n;
        }
        c--;
    }

    while(arr[i]>=0){
        crr[c] = arr[i] + r;
        r = 0;
        c--;
    }
    while(brr[j]>=0){
        crr[c] = brr[j] + r;
        r = 0;
        c--;
    }
    if(r!=0){
        crr[c] = r;
    }
}
cse
  • 4,066
  • 2
  • 20
  • 37
  • 4
    You use [variable-length arrays](https://en.wikipedia.org/wiki/Variable-length_array) which are invalid in C++ (though unfortunately some compilers allow it as an *extension* of the language). Use [`std::vector`](https://en.cppreference.com/w/cpp/container/vector) instead. – Some programmer dude Dec 10 '18 at 09:43
  • 4
    I also recommend you get [a couple of good books](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list/388282#388282), and learn about *scopes*. You have three different variables named `crr`, all defined in different scopes. Some of them go out of scope (and their life-time will end) before you use the argument named `crr`. – Some programmer dude Dec 10 '18 at 09:44
  • 6
    *" I also wanted to know if this code could be optimized"* - How about starting out with formatting to make it readable? – Filburt Dec 10 '18 at 09:45
  • 3
    ... and comments, and useful variable names... – paddy Dec 10 '18 at 09:48
  • 1
    By the way, this is no runtime error, but a compile time one... – Aconcagua Dec 10 '18 at 11:04

3 Answers3

3

You declare variables in a block scope, i.e. inside { ... }, and these variables are visible only within this block:

if(size1>size2){ 
  int crr[size1+1];
  int c = size1;
}
else{
  int crr[size2+1];
  int c = size2;
}
...
crr[c] = ...  // neither crr nor c are valid here any more

BTW: C++ does not support variable length arrays like int crr[size1+1] (when size is not a compile-time-constant).

To overcome this, write...

int *crr;
int c;
if(size1>size2){ 
  crr = new int[size1+1];
  c = size1;
}
else{
  crr = new int[size2+1];
  c = size2;
}

...

delete[] crr;
Stephan Lechner
  • 34,891
  • 4
  • 35
  • 58
1

About scope issue: see Stephan's answer.

I also wanted to know if this code could be optimized

By use of std::vector. OK, the following is only a fine option if you can use vectors outside as well – copying the raw arrays into vectors wouldn't be efficient either... But if you can, then you might like this variant:

template <typename T> // optional: you're more flexible if you make a template of...
void sumOfTwoArrays(std::vector<T> const& va, std::vector<T> const& vb, std::vector<T>& vr)
{
    vr.resize(std::max(va.size(), vb.size() + 1));
    int carry = 0; // renamed r to something more meaningful

    // these pairs will help to avoid code duplication later
    std::pair pa(va, va.rbegin());
    std::pair pb(vb, vb.rbegin());
    auto ir = vr.rbegin();
    while(pa.second != pa.first.rend() && pb.second != pb.first.rend())
    {
        // just skip the if/else:
        // assume you have arbitrary number, the else case will be entered anyway
        // in 50 % of the cases - in the other 50 %, the else branch calculates
        // the correct result, too; and on most modern machines, the branch is
        // rather expensive, so you result in easier code and have quite a good
        // chance to even perform better...
        carry += *pa.second + *pb.second;
        *ir = carry % 10;
        carry /= 10;
        ++ir, ++pa.second, ++pb.second;
    }

    // avoiding of two identical while loops: iterate over the two pairs...
    for(auto p : { pa, pb })
    {
        // just loop over, if we are already at the end, won't enter...
        while(p.second != p.first.rend())
        {
            // STILL need to continue calculating the carry!
            // imagine we have set it and ciphers following are all 9!
            carry += *p.second;
            *ir = carry % 10;
            carry /= 10;
            ++ir, ++p.second;
        }
    }
    // assign either 0 or 1...
    *ir = carry;
}

Variant: instead of assigning 0, you could erase first element at the very end:

if(carry == 0)
{
    vr.erase(vr.begin());
}
else
{
    *ir = carry;
}

Note that this will move all the elements one position to front. On the other hand, if you repeatedly add vectors already containing a leading zero, you might prepend another one again and again without need, if you don't drop it again.

You wouldn't experience any of these issues if you inverted the order of digits in the vector, having least significant one at position 0 (you'd exchange rbegin() and rend() with begin() and end(), but would use the former for printing data to display...). Erasure at the end would be an O(1) operation then:

if(carry == 0)
{
    vr.erase(std::previous(vr.end())
}
// ...

All this above will only work as expected if you keep your vectors normalised (i. e. all digits in between 0 and 9 inclusive). You might consider packing the vector into a separate class such that the data is hidden away from the user and only can be modified in controlled manner (assume you have a fine vector, but a user does v[7] = -1012...).

Aconcagua
  • 24,880
  • 4
  • 34
  • 59
0

A runtime error suggests that it is a memory issue i.e. you are writing to some memory which is not allocated to be used by your code. So, as mentioned by other contributors, you should allocate proper memory for your arrays.

Following is modified version of your code which is working fine. You can see it working here:

void sumOfTwoArrays(int arr1[], int size1, int arr2[], int size2, int sumArr[])
{
    int maxLen;
    int* tArry;
    int l;
    if(size1>size2) { maxLen = size1; tArry = arr1; l = size1 - size2; }
    else { maxLen = size2; tArry = arr2; l = size2 - size1; }

    int carry = 0;

    while(size1 && size2){
        carry += arr1[--size1] + arr2[--size2];
        sumArr[maxLen--] = carry%10;
        carry /= 10;
    }

    while(l){
        carry += tArry[--l];
        sumArr[maxLen--] = carry%10;
        carry /= 10;
    }
    sumArr[maxLen] = carry;
}

Calling code looks something like this:

... 
int a[] = {9,9,9,9,9};
int b[] = {1};
int l1 = sizeof(a) / sizeof(int), l2 = sizeof(b)/sizeof(int);
int l3 = ((l1 > l2) ? l1 : l2) + 1;
int *c = new int[l3];
sumOfTwoArrays(a, l1, b, l2, c);
...
delete [] c;
... 
cse
  • 4,066
  • 2
  • 20
  • 37