1
class awesomeClass {
    std::list<A> myList;
    //...
    void fillList();
};

awesomeClass::awesomeClass() {
    fillList();
}

void awesomeClass::fillList(){
    //...
    foreach(A a, otherList)
        myList.push_back(a); //add to list
}

Compared to :

awesomeClass::awesomeClass() {
    myList = fillList();
}

std::list<A>& awesomeClass::fillList() {
    //...
    std::list<A> ret;
    foreach(A a, otherList)
        ret.push_back(a); //add to list
    return ret;
}

This is just a class with an std::list as a member variable and a function which fills this list. The first fills it inside the function and the second one returns a list and assigns it to the member list.

Which of these is better style? The one which fills the list inside the function or the one which returns a list? Or is there an even better solution?

migrated from codereview.stackexchange.com Nov 28 '13 at 18:07

This question came from our site for peer programmer code reviews.

  • These data members and functions are supposed to be private, right? – Jamal Nov 28 '13 at 17:38
  • @Jamal yeah. Would it be different if both or one of these was public? – Davlog Nov 28 '13 at 17:41
  • 2
    myList, at least, should be private since it's a data member. I know that's not the main point here, though. I just wanted to clarify that. – Jamal Nov 28 '13 at 17:42
  • Moreover, this question could be better suited for Programmers. I could migrate this for you. – Jamal Nov 28 '13 at 18:02
  • 8
    Your second fillList() function returns a reference to a local variable which gets destroyed when it goes out of scope. That's never a good idea. – Blastfurnace Nov 28 '13 at 18:49
3

In object orientied programming, classes have

  • State, which is represented by instance variables, and
  • Methods, which change the state

So your first approach is correct where you call a method that sets the instance variable.

You should return a value only when that is not represented by any instance variable or it is a public method called by consumer, which doesn't have access to the instance variable.

1

What is the scope and lifetime of the data that you are putting into this list? Does it need to live for the entire lifetime of an instance of awesomeClass?

Are you trying to avoid re-generating the list frequently - i.e. there is value in caching the data in individual instances? Or are you just being lazy and wanting to declare all your variables in the class definition?

Lets make our theoretical member more tangible:

// 2Kb+ structure
struct Data {
    char m_vendorString[1024];
    char m_productString[1024];
    uint32_t m_vendorId;
    uint32_t m_productTypeId;
    uint32_t m_productId;
};

Lets try a few variations of how we might encapsulate an interface that we would retrieve large numbers of this large object from, in the form of pulling them from a database:

class DatabaseA1 {
    std::vector<Data> m_productQuery;
    std::vector<Data> m_vendorQuery;
public:
    // .. various functions
    const std::vector<Data>& getVendorProducts(uint32_t vendorId);
    const std::vector<Data>& getProductVendors(uint32_t productTypeId);
};

class DatabaseA2 {
    std::vector<Data> m_results;
public:
    // .. various functions
    const std::vector<Data>& getVendorProducts(uint32_t vendorId);
    const std::vector<Data>& getProductVendors(uint32_t productTypeId);
};

class DatabaseB {
public:
    // .. various functions
    std::vector<Data> getVendorProducts(uint32_t vendorId);
    std::vector<Data> getProductVendors(uint32_t productTypeId);
};

class DatabaseC {
public:
    // .. various functions
    bool getVendorProducts(std::vector<Data>& into, uint32_t vendorId);
    bool getProductVendors(std::vector<Data>& into, uint32_t productTypeId);
};

Consider that we do something like the following pseudo-code:

Database db(parameters);

for (possibly hours or days) {
    // get user input
    if (option == GetVendorProducts)
        getVendorProducts(id);
    else if (option == GetProductVendors)
        getProductVendors(id);
    else ...
    doOtherStuff();
}

Lets say the result sets contain a few thousand Data items - so a few Mb of data.

That data is going to persist until it goes out of scope or until you call something to release it.

In the case of A1 you're going to have this problem for each query type - if you don't provide a mechanism to release the vectors, and someone queries products and then vendors, you're going to have 2x the number of Mbs of memory used. In the case of A2, you have forced yourself to retain information about what data you are keeping.

For A1 and A2 you're going to have to add an accessor that will tell the member(s) to release their memory, which is fiddly and increases the footprint of your API.

In the case of B, the compiler can use return value optimization (RVO) to make this efficient.

In the case of C, you avoid dependency on RVO and make it much easier to robustly re-use a vector that the end user provides.

There are times when a pattern like A1 or A2 are called for, if you conceivably might have to re-build the data a lot of times, and if your objects themselves actually depend on the data (which is not the same thing as merely having a lot of helper functions that can work with the data).

One example, of course, would be if you are specifically trying to encapsulate a set of data behind an abstraction, e.g.

class DatabaseResults {
    DatabaseQueryType m_queryType;
    std::string m_srcQuery;
    std::vector<Data> m_results;
public:
    bool populateResults(DatabaseQueryType queryType, std::string srcQuery);
    const std::vector<Data>& getResults() const { return m_results; }
};

What is the result sets relationship to the rest of our class? If it is solely an output then we probably don't want it in the class. If we return a reference to it, what promises do we make to callers about it's lifetime.

class Fnar {
    std::list<ImportantThings> m_list;
public:
    void makeAList();
    std::list<ImportantThings>& getThings() { return m_list; }
    void mowLawn() {
         if (m_list.empty())
             crashHorribly();
         m_list.clear();
         std::cout << "lawn mowed\n";
    }
};

Fnar fnar;
fnar.makeAList();
auto& list = fnar.getThings();

// either
list.clear(); // take that, fnar.
list.mowLawn(); // horrible crash happens here.

// or
list.mowLawn(); // horrible crash happens here.
for (auto& it: list) {
    std::cout << it << "\n";
}

Neither of those two cases works right.

If fnar never uses the list itself, then we really ought to hoist the list out and let the caller own it. If there is some other value to our retaining the list - perhaps we are persisting some concept of ownership on some down-stream data we used to build the list - then there is value in having the list in-class and providing callers with a const-only view of it:

    const std::list<ImportantThings>& getThings() const { return m_list; }

Try to avoid empire building in classes - it's easy to throw everything into the instance so that in the debugger you can see every variable that's been used in reaching the current state or you need to serialize them later and retain all of that state.

If you don't, then only own what you absolutely need to. It'll make the code simpler and it'll help localize resources which will make their role in the code easier to understand.

0

If all you are doing is copying a list, I would use the range constructor.

c++11

AwesomeClass::AwesomeClass (const std::list &otherList) 
: myList (std::begin (otherList), std::end (otherList))
{
}


c++

AwesomeClass::AwesomeClass (const std::list &otherList) 
: myList (otherList.begin (), otherList.end ())
{
}

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.