5
\$\begingroup\$

I want to have a class that stores data from an input file, for use in a simulation program. It is like a struct but may require some small functions for accessors.

A seperate class reads all the data from a text input file. I want to store most of this data in the above mentioned object. Therefore this class needs to be able to set the data variables.

My current approach is to give the loader class friend access, so that these variables can be set. No other classes should be able to set the data, therefore I am not using setters.

Is this appropriate friend class usage?

I have included a demonstration example of the situation, as simple as possible:

In reality the data holder I have called "Pod" here, holds 10 to 20 different strongly related variables.

#include <iostream>
class Pod {
 // holds the data that will be used later in simulation
 friend class Loader;
 public:
    double get_angle() {return angle_ * 3 / 180;};
 protected:
    double angle_;
};

class Loader {
 // loads data from text file.
 public:
    bool CheckFile() {
        // checks file:
        return true;
    }
    Pod ReadData() {
        double a = 90; // this line simulates reading from file.
        Pod pod;
        pod.angle_ = a;
        return pod;
    }
};

void DoesThingsWithData(Pod data) {
    std::cout << data.get_angle() << std::endl;
}

int main() {
    Pod my_data;
    {
        Loader loader;

        if (loader.CheckFile()) {
            my_data = loader.ReadData();
        }
    }
    // do things with data
    DoesThingsWithData(my_data);
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Why not have does Pod not have a constructor. Then no need for a friend. Pod ReadData() { return Pod(<readFromFile>);} \$\endgroup\$ – Martin York Oct 31 '14 at 13:24
4
\$\begingroup\$

Personally, I don't have major objections with that design. It gets the job done in a fairly straightforward way. But it does creates coupling, which might become a nuisance to you as the project grows. So another option with less coupling, as mentioned in comments, would be defining a default parameterized constructor:

class Object {
public:
    Object(<several params>);
private:
    <the data>
};

class Loader {
public:
    Object LoadObject()
    {
        // many fields of data...
        int a    = ...
        double b = ...
        std::vector<int> c = ...

        return Object(a, b, std::move(c), ...);
    }
};

In C++11, you also don't have to worry about unnecessary copies of complex objects such as a std::vector. You can make the constructor take a move ref, e.g.: std::vector<int> && vec and apply std::move in the call site.

\$\endgroup\$
3
\$\begingroup\$
  • protected == public. When you think in terms of encapsulation, public means that anyone can access that element and if you change it it breaks an arbitrary number of people's code. If you make it protected instead people can still access the data by inheriting, so changing a protected member will also break an unknown amount of code, which makes public and protected equal in encapsulation. Since the destructor of Pod is not virtual it is questionable at best to have protected members at all.

  • We are mostly responsible adults. It is neither necessary nor feasible to protect your code from malicious misuse. People can just change your header and access everything. Limit yourself to preventing accidental misuse of the code, that is the maximum you can do anyway. And I do not see how someone would use a setter accidentally and then be surprised the data changed, so there is no reason to prevent access.

  • Pod starts up uninitialized. Just like justanothercoder I would add a constructor. Instead of doing the angle calculation when getting the value I would do it after I read it from the file. It is confusing to store something and get something different later. This kind of makes Pod obsolete, a double will work just fine. You can still achieve the read only property with const.

  • Pod is a bad name for this, maybe call it Angle instead.

EDIT: I kind of forgot to answer the actual question... No, do not use friend. It is not necessary to manage access at that level of detail. Try to avoid inheritance, it is complicated and does not scale well. If you must have inheritance try to hide it using NVI or type erasure.

\$\endgroup\$
  • \$\begingroup\$ so what I am taking from this, is that I should use a constructor with the 10 or so required variables, and make the variables const so that the object is immutable. Note that I am not using any inheritance, friend was just a way for one object to be able so set the fields, while to all other objects, this data holder would appear immutable. \$\endgroup\$ – windenergy Oct 31 '14 at 10:18
  • \$\begingroup\$ the reason the "angle calculation" performs an operation is that it is just a simplification of a my real situation. \$\endgroup\$ – windenergy Oct 31 '14 at 10:20
  • \$\begingroup\$ @windenergy Making member variables const is usually a bad idea, because assignment and swap cannot work anymore. Instead make a particular instance const. Using constructors is the way to go. \$\endgroup\$ – nwp Oct 31 '14 at 10:32
1
\$\begingroup\$

Personally I prefer objects that know how to read/write themselves from a stream.

class Pod
{
    public:
        Pod()              // Should always have some form of constructor
            : angle_(0)    // When you have POD data otherwise it has random values.
        {}
        // I assume this calculation is an artificat of this being
        // a contrived example. Otherwise I would store the value
        // you want to return rather than re-calculate every time.
        double get_angle() const   // Note the const
        {                          // Its a non mutating function.
             return angle_ * 3 / 180;
        }

    // Protected buys you very little.
    // Prefer private in nearly all situations.
    // If you must have access from derived members provide
    // protected get/set methods as needed (no need to make them public).
    private:
        double angle_;

    // Tightly coupled:  friend functions. 
    // Basically extend the public interface of your class.
    // Thus increasing your ability to maintain encapsulation.
    //
    friend std::istream& operator>>(std::istream& str, Pod& data)
    {
        return str >> data.angle_;
    }
    friend std::ostream& operator<<(std::ostream& str, Pod const& data)
    {
        return str << data.angle_ << " ";
    }
};

Usage then becomes much simpler:

int main() {
    Pod my_data;

    std::ifstream  file("MyFile");
    if (file)
    {
        file >> my_data;
    }

    // Note:
    // With no constructor and a file that failed.
    // This function now has a Pod with random data in it.
    // Worse: Trying to read that data before initialization
    //        is actually undefined behavior. So make sure your
    //        class initializes its members on construction.
    DoesThingsWithData(my_data);

    // Main is special.
    // No need to return a value (the compiler will add `return 0;` for you.
    // Not returning a value is an indication to a reader
    // your code has no failure states and thus does not
    // need an explicit return.
    return 0;
}

Even better is that by defining the operator>> you can use a whole bunch of standard utilities to help you.

int main()
{
     std::ifstream       myFile("data");

     // Read a file of Pod values directly into a vector.
     // Note: this works because vector has a constructor that
     //       takes two iterators. Here we have two iterators
     //       into a stream (of type Pod).
     std::vector<Pod>    data(std::istream_iterator<Pod>(myFile),
                              std::istream_iterator<Pod>());


     // Copy a vector of Pod to the output.
     std::copy(std::begin(data), std::end(data),
               std::ostream_iterator<Pod>(std::cout));
}
\$\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.