Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have a fairly large software project written in c++.

In there, there is a class foo which represents a structure (by which i don't mean the programmers struct) in which foo-objects can be part of a foo-object.

Here's class foo in simplest form:

class Foo
{
    private:
        std::vector<unsigned int> indices;
    public:
        void addFooIndex(unsigned int);
        unsigned int getFooIndex(unsigned int);
};

Every foo-object is currently stored in an object of class bar.

class Bar
{
    private:
        std::vector<Foo> foos;
    public:
        void addFoo(Foo);
        std::vector<Foo> getFoos();
}

So if a foo-object should represent a structure with a "inner" foo-object, I currently do

Foo foo;
Foo innerFoo;
foo.addFooIndex(bar.getFoos().size() - 1);
bar.addFoo(innerFoo);

And to get it, I obviously use:

Foo foo;
for ( unsigned int i = 0; i < foo.getFooIndices().size(); ++i )
{
    Foo inner_foo;
    assert( foo.getFooIndices().at(i) < bar.getFoos().size() );
    inner_foo = bar.getFoos().at(foo.getFooIndices().at(i));
}

So this is not a problem. It just works. But it's not the most elegant solution.

I now want to make the inner foos to be "more connected" with the foo-object. It would be obviously to change class foo to:

class Foo
{
    private:
        std::vector<Foo*> foo_pointers;
    public:
        void addFooPointer(Foo*);
        std::vector<Foo*> getFooPointers();
};

So now, for my question: How to gently change this basic class without messing up the whole code? Is there a "clean way"?

share|improve this question
1  
Are you actually copying Foo when you take it in and out of Bar as the code suggests? Right now addFoo will make a a copy, and the complete collection will be copied to create the return value of getFoos(). That would be rather unusual, so I'm wondering if thats actually whats going on. –  Winston Ewert May 9 '12 at 0:02
    
actually, right now it's copies that I store, but that's just a small design mistake ;-) –  stefan May 9 '12 at 7:55
5  
Your interface is simply wrong – you shouldn’t bleed implementation details all over the place: getFoos is almost certainly a bad idea; the client of Bar has no business directly accessing this collection (also, you are returning a copy – is that intentional?) –  Konrad Rudolph May 9 '12 at 16:58
    
Actually, in a previous version, this was all not only just working, but necessary. It's just like that for the sake of simplicity –  stefan May 9 '12 at 20:16
2  
I would avoid using pointers in interface, use references. This is a general statement, not just to help the redactor. –  richard Jul 14 '12 at 21:37

3 Answers 3

I'll take a stab at this.

It all boils down to what kind of memory management you want/need. You can either store pointers in Foo, or Foo objects. Since you said you do not need bar, which currently 'owns' the Foo's memory-wise, I suggest you put them directly in Foo:

class foo
{
public:
  add_child(foo child); //Foo by value. Utilize 'move' semantics if copy is not possible.
  const std::vector<foo>& get_children(); //Get a reference to the children. Could use iterators or whatever.

private:
  std::vector<foo> foos;
}

Usage:

foo parent;
foo child;
foo grand_child;
child.add_child(std::move(grand_child)); //move semantics.
parent.add_child(child);  //no move semantics.

parent.traverse(root);

void traverse(foo root)
{
    root.discombobulate_the_parafunctors(); //act on root

    auto children = root.get_children();        
    std::for_each(children.begin(), children.end(), traverse);
}

This is what I would try to move towards, but you said you want to do it 'gently' - I guess you mean incrementally, without breaking things? Since ownership is transferred from bar to foo in the example above, it will not be possible to use both at the same time. The pointer approach, however, can be layered on top of the existing bar implementation.

If I really wanted to 'gently' switch, I'd make a class called something like foo_tree_view, which is like your first approach, with pointers, and doesn't assume ownership. I'd build it from a given bar instance, and then use it in client code.

std::vector<foo_tree_view> bar_view;

class foo_tree_view
{
public:
  foo_tree_view(foo this_foo, bar source);
  std::vector<foo*> get_children();
private:
   ...
}

At this point, you start gently changing all places to use this new foo_tree_view class. When all code uses it, you change to implementation of foo_tree_view to use the above implementation, without 'bar', and rename it to foo.

Hope you'll make some sense out of this answer!

share|improve this answer

What I'd do:

  1. Add a member variable Bar * bar and int index to Foo.
  2. Have Bar::addFoo set foo->bar = this and foo->index = foo.size()
  3. Implement Foo:getFoo on Foo to use the getFooIndex and bar to return a foo pointer
  4. Implement Foo:addFoo similiarily

Then you should be able to use both APIs simultaneously. You can deprecate the index based apis and slowly remove them.

share|improve this answer

I don't think having the foo object be responsible for its index in a container is a good idea. It should be blissfully unaware of the container(s) it is in. Maintaining these indices is the job of the container (if it is applicable for that container).

Your current code requires a foo to retrieve the inner foos from bar so why not just retrieve them directly from foo?

for (unsigned int index = 0; index < foo.getFooPointers().size(); ++index)
{
    Foo* inner_foo = foo.getFooPointers().at(index);
}

Why do you need the inner foos stored in bar? If there is some situation where you want to loop through all foos, including inner ones, you could create an iterator.

As a side note I would probably call the member function something like getInnerFoos() or getChildren() instead of getFooPointers(). The signature already tells you that it returns pointers of type foo.

share|improve this answer
    
Basically I don't need the foos in the bars. this was the work-around. The given method names are just symbolic and do not actually show what I'm doing ;-) I think you missed the question a bit: I know that pointers are better. I want to migrate to this, but I can't think of an elegant method to do so –  stefan May 8 '12 at 23:24
    
I would probably deprecate the index management methods, add comments to them advising of the alternatives and educate the team on how to use getFooPointers() and any other new methods or objects you provide like maybe an iterator. Initially this could be done in an email to the team and then during code reviews it could be enforced. –  Gary Buyn May 9 '12 at 0:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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