14

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?

  • 13
    If you feel like doing this, it's hinting at a bad design. The properties of the class probably belong somewhere else. – bitsoflogic Dec 9 '18 at 17:53
  • 11
    Snippet is too small. This level of mixed abstractions does not occur in snippets of this size. You can also fix this with some good text on design considerations. – Joshua Dec 9 '18 at 20:18
  • 16
    I honestly don't understand why you would even do this. What do you gain? Why not just have a no-arg add method that operates on its internals directly? Just, why? – Ian Kemp Dec 10 '18 at 9:41
  • 2
    @IanKemp Or have an add method that takes arguments but doesn't exist as part of a class. Just a pure function for adding two arrays together. – Kevin Dec 10 '18 at 19:11
  • Does the add method always add arr1 and arr2, or can it be used to add anything to anything? – immibis Dec 11 '18 at 4:33
33

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.

  • Indeed, vector<int> would be more suitable; length would then be useless. Alternatively const int len, since variable length array is not part of C++ standard (although some compilers support it, because it's an optional feature in C). – Christophe Dec 9 '18 at 18:14
  • 5
    @Christophe Author was using fixed size array, which should be replaced with std::array which doesn't decay when passing it to parameters, has a more convenient way to get its size, is copyable and has access with optional bounds checking, while having no overhead over C style arrays. – Cubic Dec 10 '18 at 0:39
  • 1
    @Cubic: whenever I see code declaring arrays with an arbitrary fixed size (like 100), I am pretty sure the author is a beginner who has no idea about the implications of this nonsense, and it is a good idea to recommend him/her changing this design to something with a variable size. – Doc Brown Dec 11 '18 at 6:17
  • The arrays are fine. – Lightness Races in Orbit Dec 11 '18 at 11:02
  • ... for those who did not understand what I mean, see also Zero One Infinity Rule – Doc Brown Dec 11 '18 at 11:38
48

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.
7

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.

1

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.

0

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.

0

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.
0

This is a classic "C with classes" approach to C++. In reality, this isn't what any seasoned C++ programmer would write. For one, using raw C arrays is pretty much universally discouraged, unless you're implementing a container library.

Something like this would be more appropriate:

// Don't forget to compile with -std=c++17
#include <iostream>
using std::cout; // This style is less common, but I prefer it
using std::endl; // I'm sure it'll incite some strongly opinionated comments

#include <array>
using std::array;

#include <algorithm>

#include <vector>
using std::vector;


class MyClass {
public: // Begin lazy for brevity; don't do this
    std::array<int, 5> arr1 = {1, 2, 3, 4, 5};
    std::array<int, 5> arr2 = {10, 10, 10, 10, 10};
};

void elementwise_add_assign(
    std::array<int, 5>& lhs,
    const std::array<int, 5>& rhs
) {
    std::transform(
        lhs.begin(), lhs.end(), rhs.begin(),
        lhs.begin(),
        [](auto& l, const auto& r) -> int {
            return l + r;
        });
}

int main() {
    MyClass foo{};

    elementwise_add_assign(foo.arr1, foo.arr2);

    for(auto const& value: foo.arr1) {
        cout << value << endl; // arr1 values have been added to
    }

    for(auto const& value: foo.arr2) {
        cout << value << endl; // arr2 values remain unaltered
    }
}

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.