2
\$\begingroup\$

As part of a question designed to help us understand the relationship between pointers and arrays in C, I've been asked to write an inner product function that doesn't use any array subscripting. Here's what I came up with, but it looks like the kind of complicated 'clever' coding that we've traditionally been told NOT to write. Any feedback on it, or how it could be done better / more efficiently would be much appreciated.

int inner_product(const int *a, const int *b, int n) {

    const int *p, *q;
    int result = 0;
    for(p = a, q = b; p < a + n, q < b + n; p++, q++) {
            result += *p * *q;
    }
    return result;
}

Edit - Inner product is simply summing the product of the indexes, so a[1,2,3] b[2,3,4] would be 1*2 + 2*3 + 3*4

\$\endgroup\$
3
\$\begingroup\$

I have a few comments on style. (By which I really mean, 100% opinion... :D)


One declaration per line

A lot of people strongly disagree with this, but I find it much easier to read code that has one declaration per line.

const int *p;
const int *q;

Is a lot clearer to me than:

const int *p, *q;

Though really, the only time I've vehemently opposed to it is if both variables are not the same in terms of a pointer or value:

const int *p, q;

Is a harder to read and more prone to errors than:

const int *p;
const int q;

n

I would name the n paramter something more meaningful. I would consider calling it size or num.

It's fairly apparent that n is meant to be the number of elements, but two or three extra keystrokes seems worth the extra clarity.


Possible implementation

I would probably write the function something like this:

int inner_product(const int *a, const int *b, int num)
{

    const int* end = a + num;

    int sum = 0;

    while (a != end) {
            sum += (*a) * (*b);
            ++a;
            ++b;
    }

    return sum;

}

Since the two pointers are incremented at the same time, there's no reason to check both.

This should be a tiny bit more efficient (well... probably not once the compiler does it's magic optimisations on the original), and I think it's a bit clearer.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for that. I've tested all of the suggestions here, will add a comment at the end of the original question. \$\endgroup\$ – Saf Aug 6 '12 at 17:23
  • \$\begingroup\$ Making this the answer as it runs a bit faster than the other three. Is it because it only has one comparison in the while loop possibly? \$\endgroup\$ – Saf Aug 7 '12 at 21:40
1
\$\begingroup\$

Just a point, that code will error on compilation: p++ and q++ are not permitted operations if you have a const int.

Now, to write it more cleanly (I know next to nothing about inner products, and google hasn't been much help):

int inner_product(const int *a, const int *b, int n)
{
 int *p = a, *q = b;
 int result = 0;
 while( (p < a + n ) && (q < b + n ) ) /*|| might be required here, but a comma probably (never come across it) won't work*/
 {
        result += *p * *q;
        p++; q++;
 }
 return result;
}

The if loop you used was messy, hard to read, and too 'clever'. This way, it is clear what your code is doing. To be even more easy to read, I'd surround my calls to the pointers *p in parenthesis: (*p), but that's a matter of personal taste.

Edit: Since the definition of inner product has now been defined for me, I can safely say that your algorithm is the most clean I could have come up with, and is the fastest by far (linear time).

EDIT: Where I say int *p = a, *q = b;, it would be more proper to say const int *p = a, *q = b;

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks for that, that code works and does the same job. It does however throw warnings about discarding the const qualifier for the line int *p =a, *q =b. That was the reason I put const in front of them in the first place, is this a warning one should ignore? Also, my version does compile - from what I understand const int *p protects the value it points to but not p itself, hence the ++ compiling & working. I could be missing something here. \$\endgroup\$ – Saf Aug 5 '12 at 15:23
  • \$\begingroup\$ I wrote the program up with a sample main(), and learned that should I remove the const declarations from the function header, the warnings went away. To test the const not affecting pointer arithmetic, I googled, and found Stack Exchange questions where you are correct. You may continue to use the const int as you were before, it was not necessary for me to remove them. \$\endgroup\$ – Aviator45003 Aug 5 '12 at 21:27
  • \$\begingroup\$ Oh, as for the warnings, I'd just ignore them in my own code. That particular warning was a pointer-mismatch, and I could see there was none of any significance present. \$\endgroup\$ – Aviator45003 Aug 5 '12 at 21:33
  • 1
    \$\begingroup\$ @T.C. Sorry, but -1. "Where I say ... it would be more proper to say ..." It's not more proper that way; it's correct that way. In addition, your opening line about incrementing the pointers is wrong, and there's no reason to make copies of a and b. \$\endgroup\$ – Corbin Aug 6 '12 at 4:43
  • \$\begingroup\$ One more question on your edit though, "proper to say const int *p = a, *q = b", would I be right in thinking that the second declaration, *q = b will not be treated as having the const qualifier? \$\endgroup\$ – Saf Aug 6 '12 at 17:27
0
\$\begingroup\$

I agree with all the points from Corbin but i personally would use a simple for loop instead as this is one of the more common ways to iterate through arrays:

int inner_product(const int *a, const int *b, int size) {

  int sum = 0;
  int i = 0;

  for (i=0; i<size; ++i) {
    sum += *(a+i) * *(b+i);
  }

  return sum;
}
| improve this answer | |
\$\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.