views:

132

answers:

8

I am try to make the myFunction give me a sum of the values in the array, but I know I can not use a return value, and when I run my program with the code as so all I get is a print out of the values and no sum why is that?

void myFunction (int i) {
int total = 0;
total += i;
cout << total;
}


int main() {
int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

for_each( array, array+10, myFunction);


return 0;
}
A: 

The total is a local variable. It will be destroyed once the myFunction finished processing one data.

The typical way to have a state is to make myFunction a function object (a struct that overloads the () operator).
The dirty way is to make total a global variable.
In your case I'd recommend you use the accumulate function instead (assuming that cout << total is just for debugging).

KennyTM
A: 

Your total is local at each function call. It's initialized with 0 at every iteration. You could just declare it global (or pack into a parameter, etc.)

Vlad
Adding external global state to a function is not a good idea. It makes the code hard to test and hard to make parallel and tightly couples the code to the external state.
Martin York
Yes, indeed. A better idea would be to pack a sum into a parameter given to the accumulating function call at each iteration.
Vlad
+8  A: 

You really need a functor to store state between iterations:

struct Sum
{
    Sum(int& v):  value(v) {}
    void operator()(int data) const { value += data;}

    int& value;
};

int main()
{
    int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    int total = 0;
    std::for_each( array, array+10, Sum(total));

    std::cout << total << std::endl;
}
Martin York
What's the advantage of a 'functor' over using a static variable?
reemrevnivek
@reemrevnivek: clarity, not polluting global namespace, possibility to run the same calculation simultaneously from different threads, correct result on the second run :)
Vlad
@reemrevnivek: You can not access a function static variable outside the function. If you are talking about a generic static variable (i.e. globals) then you are introducing external state into your function (which makes the code hard to test/hard to make parallel is generally bad as with all global variables). In general your function should only use what it is passed using external state makes everything harder.
Martin York
@reemrevnivek: you really need to add values to the total as its called by for_each, but then retrieve the value afterwards, so the 'adder' and 'retriever' both need access to a common variable (that should generally be hidden from the rest of the world).
Jerry Coffin
You may run into trouble on some implementations when the compiler pukes over trying to generate an assignment operator and/or copy constructor. The for_each algorithm is free to use either or both.
Noah Roberts
The functor supplied to for_each is supposed to be assignable, yours is not.
Eugen Constantin Dinca
hehehe. I gave a very similar but more complete and accurate answer the previous time this poster asked the same question. Here he accepts it, over there he ignores it. Go figure.
Noah Roberts
@Eugen Constantin Dinca: What reference are you using to come to that conclusion?
Martin York
@Martin York: mostly the SGI docs (http://www.sgi.com/tech/stl/for_each.html and below regarding the requirements for the UnaryFunction). Also that `for_each` returns the functor and it would be "natural" (even with move semantics - assuming a class with a reference member is move constructible) to use it as such: `Sum s; s = for_each(...,s);`.
Eugen Constantin Dinca
@ Eugen Constantin Dinca: I was using the same reference. You are reading too much into it. That is not a requirement. The returning of the value is a feature of the for_each() that you can use it does not make it a requirement on your object to make it assignable. Though if you want to copy the result out of for_each then it will need to be (thus it is a feature that can be utilized). A requirement is a constraint on the object (type) that would stop it from working properly if the requirement was broken. Not true in this case. It also still works like this Sum a = std::for_each(..., Sum(x));
Martin York
@Martin York: I don't get how your implementation of `Sum` still works like `Sum a = std::for_each(..., Sum(x));`...
Eugen Constantin Dinca
A: 

myFunction is being called each time and total is local to the function... try marking total as static instead:

static int total = 0
Wyatt Anderson
How do you get the total out of the function? Say to print after the loop is finished?
Martin York
Oh. Right. Your solution takes care of that rather elegantly :)
Wyatt Anderson
+1  A: 

When you declare a variable (i.e. int total) it exists for the duration of its scope (usually equivalent to the nearest surrounding pair of { and }. So, in your function myFunction, total ceases to exist when the function returns. It returns once per call--once per element in your array, that is. In order to actually sum its values (or otherwise preserve a variable beyond the end of myFunction, you must give it a broader scope.

There are two relevant ways to do this. One is a "good" way, and one is an "easier-but-badly-styled" way. The good way involves a functor or context object--@Martin has already posted an example. The "bad" way is marking int total as static. It'll work the first time you use it, if your code is single-threaded... and then never again. If somebody suggests it... don't do it. :)

Jonathan Grynspan
This program is 8 lines. Why worry about multithreading at this point? It will only be confusing.
reemrevnivek
Indeed. Hence ***don't do it*** in bold/italics.
Jonathan Grynspan
@reemrevnivek: the reason you worry about multithreading at this point is because the gave what's called a *minimal example* of his problem. We assume that he has a real codebase out there that's demonstrating a *similar* problem to the one the code showed. Besides that, you should be in the habit of writing code that's thread safe when there's no significant downside to doing so.
Ken Bloom
A: 

You need to make the total persistent across function calls. You also need to access the result separately from adding the intermediate results to it, which rules out (at least straightforward use of) a function at all -- you really need a class instead.

Jerry Coffin
A: 

You reset the value of 'total' to 0 each time you call the function.

Declare it 'static'

void myFunction (int i) {
    static int total = 0;
    total += i;
    cout << total;
}

EDIT:

Alternatively, if you want to access the value of 'total' later, you will need to either use a global variable (of some kind! Could be in a class or functor! don't flame me!), or just use a for loop, and pass it in as a pointer (i.e., not use for_each):

void myFunction (int i, int * p_total) {
    //No initialization
    *p_total += i;
    cout << *p_total;
}


int main() {
    int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
    int total = 0;

    for(int i = 0; i < 10, i++)
       myFunction(array[i], &total);

    //total is now 55

    return 0;
}

Note: I'm a C programmer trying to learn C++. This is how I would do it (which is very C-like), it might not be the standard C++ way.

reemrevnivek
Say I don;t want to print it each time. But at the end of the loop I want to access it for another calculation. Now what?
Martin York
+1  A: 

total is a variable with automatic storage duration. Every time myFunction() is called, a new total is created and initialized to 0. You could:

  • give total static storage duration (with the static keyword), but you won't be able to assign its value to anything, because it is still local scope. A bad idea if you want to reuse this function, anyhow.
  • make total a global variable. Also a bad idea if you want to reuse this function
  • make a "functor", as described in Martin York's answer. This is the most reusable implementation

But, my chosen solution is "you're asking the wrong question" and you should be using std::accumulate():

#include <iostream>
#include <numeric>

int main() {
  int array[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

  int total = std::accumulate(array, array+10, 0);
  std::cout << total << '\n';
  return 0;
}
Steve M