Ytukyg

Here's what I mean:

class MyClass {

    int arr1[100];

    int arr2[100];

    int len = 100;



    void add(int* x1, int* x2, int size) {

        for (int i = 0; i < size; i++) {

            x1[i] += x2[i];

        }

    }

};



int main() {

    MyClass myInstance;



    // Fill the arrays...



    myInstance.add(myInstance.arr1, myInstance.arr2, myInstance.len);

}

add can already access all of the variables that it needs, since it's a class method, so is this a bad idea? Are there reasons why I should or should not do this?

There are may things with the class that I would do differently, but to answer the direct question, my answer would be

yes, it is a bad idea

My main reason for this is that you have no control over what is passed to the add function. Sure you hope it is one of the member arrays, but what happens if someone passes in a different array that has a smaller size than 100, or you pass in a length greater than 100?

What happens is that you have created the the possibility of a buffer overrun. And that is a bad thing all around.

To answer some more (to me) obvious questions:

1. You are mixing C style arrays with C++. I am no C++ guru, but I do
   know that C++ has better (safer) ways of handling arrays




2. If the
   class already has the member variables, why do you need to pass them
   in? This is more of architectural question.

Other people with more C++ experience (I stopped using it 10 or 15 years ago) may have more eloquent ways of explaining the issues, and will probably come up with more issues as well.

Calling a class method with some class variables is not necessarily bad. But doing so from outside the class is a very bad idea and suggests a fundamental flaw in your OO design, namely the absence of proper encapsulation:

• Any code using your class would need to know that len is the length of the array, and use it consistently. This goes against the principle of the least knowledge. Such dependency on the class' inner details extremely error-prone and risky.


• This would make evolution of the class very difficult (e.g. if one day, you'd like to change the legacy arrays and len to a more modern std::vector<int> ), since it would require you to also change all the code using your class.


• Any part of code could wreak havoc in your MyClass objects by corrupting some public variables without respecting the rules (so called class invariants)


• Finally, the method is in reality independent of the class, since it works only with the parameters and depend on no other class element. This kind of method could very well be an independent function outside the class. Or at least a static method.

You should refactor your code, and:

• make your class variables private or protected, unless there's a good reason not to do it.


• design your public methods as actions to be performed on the class (e.g.: object.action(additional parameters for the action) ).


• If after this refactoring, you'd still have some class methods that need to be called with class variables, make them protected or private after having verified that they are utility functions supporting the public methods.

When the intention of this design is that you want to be able to reuse this method for data which does not come from this class instance, then you might want to declare it as a static method.

A static method does not belong to any particualar instance of a class. It is rather a method of the class itself. Because static methods are not run in the context of any particular instance of the class, they can only access member variables which are also declared as static. Considering that your method add does not refer to any of the member-variables declared in MyClass, it is a good candidate for getting declared as static.

However, the safety problems mentioned by the other answers are valid: You are not checking if both arrays are at least len long. If you want to write modern and robust C++, then you should avoid using arrays. Use the class std::vector instead whenever possible. Contrary to regular arrays, vectors know their own size. So you don't need to keep track of their size yourself. Most (not all!) of their methods also do automatic bound checking, which guarantees that you get an exception when you read or write past the end. Regular array access doesn't do bound checking, which results in a segfault at best and might cause an exploitable buffer overflow vulnerability at worse.

A method in a class ideally manipulates encapsulated data inside the class. In your example there’s no reason for the add method to have any parameters, simply use the properties of the class.

Passing (part of) an object to a member-function is fine. When implementing your functions, you always have to be aware of possible aliasing, and the consequences thereof.

Of course, the contract might leave some kinds of aliasing undefined even if the language supports it.

Prominent examples:

• Self-assignment. This should be a, possibly expensive, no-op. Do not pessimize the common case for it.
  
  Self-move-assignment on the other hand, while it shouldn't explode in your face, need not be a no-op.


• Inserting a copy of an element in a container into the container. Something like v.push_back(v[2]);.


• 
  std::copy() assumes the destination does not begin inside the source.

But your example has different problems:

• The member-function does not use any of its priviliges, nor refer to this. It acts like a free utility-function which is simply in the wrong place.


• Your class doesn't try to present any kind of abstraction. In line with that, it does not try to encapsulate its innards, nor maintain any invariants. It's a dumb bag of arbitrary elements.

In summary: Yes, this example is bad. But not because the member-function gets passed member-objects.

Specifically doing this is a bad idea for several good reasons, but I'm not sure all of them are integral to your question:

• Having class variables (actual variables, not constants) is something to avoid if at all possible, and should trigger a serious reflection on whether you absolutely need it.


• Leaving write access to these class variables completely open to everyone is almost universally bad.


• Your class method ends up being unrelated to your class. What it really is is a function that adds the values of one array to the values of another array. This kind of function should be a function in a language that has functions, and should be a static method of a "fake class" (i.e. a collection of functions with no data or object behavior) in languages which don't have functions.


• Alternatively, remove these parameters and turn it into a real class method, but it would still be bad for the reasons mentioned before.


• Why int arrays? vector<int> would make your life easier.


• If this example is really representative of your design, consider putting your data in objects and not in classes and also implementing constructors for these objects in a way that doesn't require changing the value of the object data after creation.