tags:

views:

217

answers:

3
+2  Q: 

C++ design issue

+5  A: 

A::GetAInstance should be static.

Then you should be able to just do the following

if (condition)
{ 
    A::GetAInstance().CreateBInstance(name);
}
Martin
If you worked for me I wouldn't let you do this. Bad design.
SingleShot
Agreed, but without knowing what Ganesh's code is actually doing...
Martin
Why do you say it's bad design? If A is a singleton, instance should be static and CreateBinstance could perfectly be a static function.
piotr
I agree that the programming language allows you to write working software like this. It's hard to describe concisely so I will provide a link. See the section "The definition of bad design": http://www.objectmentor.com/resources/articles/dip.pdf
SingleShot
+1  A: 

Something doesn't smell right about the whole design, but its hard to say what without having more details provided. Anyway, you could do something like this which avoids a cyclic dependency but still has B call A:

class BRegistry
{
   virtual void CreateBInstances(std::string& name) = 0;
   ...
};

class A : public BRegistry
{
   public:

      virtual void CreateBInstances(std::string& name) {
         map[name] = new B(name, *this);
      }

      // Singleton stuff...

      ...
};

class B
{
   public:

      B (std::string& name, BRegistry& registry) {
         ...
      }

      void checkCondition()
      {
         if  (condition == true)
         {
            registry.CreateBInstances(name);
         }
      }

      ...
};

Basically, I extract an interface from A and have B use the interface. A passes itself to B at creation time, but from B's point of view it is the interface. No cycles.

Just a comment. 99% of the time when I see Singleton, it is an inappropriate use of the pattern. Usually it is misused as just a convenient global variable, as exhibited by all the comments that jumped to say to use it from within B.

SingleShot
You just hid the cyclic dependency with this, it's not at all gone. B still calls A which creates B, you simply made a lot harder to see. Semantic problems can not be solved with some syntactic juggling. To paraphrase you: If you worked for me I wouldn't let you do this. I rather have not-so-nice designs stand out like a pimp on miss universe's cheek, than hidden behind a few layers of mascara
Pieter
Draw it in UML and follow the arrows. There is no dependency cycle.
SingleShot
@SingleShot As I mentioned in a comment above, the dependecy is still there it has just been reduced to a dependency on an abstraction. To illustrate, B has a **uses** dependecy on BRegistry and BRegistry has a **creates** dependency on B, which is obviously a cycle.
Simon Fox
Not true my friend. BRegistry does not reference B in any way. A does. Really, get a piece of paper and draw each type (including std::string) as a box. Then draw an arrow from box to box wherever one type references another in any way in my code snippets. There will be no cycles. Really.
SingleShot
Ah sorry, didn't notice CreateBInstance signature doesn't return B.
Simon Fox
I'm not talking about a code dependency, obviously you made that disappear via an extra level of indirection. Semantically however, nothing changed, B relies on A to create a B, no matter how many functions, interfaces, classes you add to obscure that fact, it's still there. You just made it a lot harder to see...
Pieter
You are wrong that nothing changed. B can now be tested in isolation (using a Mock BRegistry). Any number of implementations of BRegistry can be substituted in place of A. Others can reuse B without having to reuse A. "Application assembly" logic has been pulled out of B and can now be placed in a dedicated area (such as main()). Modern OO is based on programming to interfaces - its not just a bunch of indirection designed to obscure things.
SingleShot
That's all syntactic mumbo jumbo. One simple question: in your design, an adaptation of the question, does a B object ask an A object to create a B object? Yes... What could possibly be replaced / tested / substituted /etc. doesn't make the slightest difference to the semantics that a B object asks an A object to create a B object. Is this really that hard to grasp?
Pieter
Pieter - I think we should agree to disagree :-)
SingleShot
+2  A: 

Much of the fluff about circular dependencies out there is confused when applied at implementation mechanisms rather than at package or module level, and some of it comes out of deficiencies in testing frameworks for Java. Because of the Java influence on the meme, solutions are phrased in the idioms of that rather than C++. It's not clear whether you're trying to remove a concrete circular dependency ( the code won't compile in C++ ) or have metaphysical objections.

One idiom for decoupling concrete circular dependencies is found in allocators in the C++ standard library.

When you declare a list so:

std::list < int, my_allocator < int > >

the allocator has a nested struct which allows access to the raw template for different specialisations so the std::list implementation can allocate node objects rather than just ints.

Assuming that you have the following requirements:

  • the registry is global to the program ( ie. you don't need more than one registry per object type, otherwise you need something like the factory pattern SingleShot proposes, though in C++ you'd more normally use template rather than virtual function polymorphism ); as such I tend to use static factory rather than singletons.
  • objects of class B should only be created by calling A::CreateInstance(name)
  • A acts as a registry, so repeated calls to create instance with the same name return the same object
  • code compiles correctly without concrete circular references
  • it is possible to replace the type of either the registry or registered type for testing

This global registry doesn't require any knowledge of the types it creates, other than that they supply a constructor taking a reference to a const std::string:

#include <string>
#include <map>

template < class T = int >
class Registry {
    static std::map<std::string,T*> map;

    public:

    static T& CreateInstance ( const std::string& name ) {
        typename std::map< std::string, T* >::iterator it = map.find ( name );

        if ( it == map.end() )
            return * ( map [ name ] = new T ( name ) );
        else
            return *it->second;
    }

    public:

    template < typename U > 
    struct rebind {
        typedef Registry<U> other;
    };
};

template  < class T >
std::map < std::string, T* > Registry<T>::map;

The corresponding registered object supplies a private constructor and has the CreateInstance function as a friend:

template < typename R = class Registry<> >
class Registered {
        typedef typename R::template rebind< Registered < R > > ::other RR;

    private:
        friend Registered<R>& RR::CreateInstance ( const std::string& name );

        explicit Registered ( const std::string& name ) {
            // ...
        }

        Registered ( const Registered<R>& ) ; // no implementation

    public:
        void checkCondition()
        {
            bool condition = 7 > 5;

            if  ( condition )
            {
                RR::CreateInstance ( "whatever" );
            }
        }

    // ...
};

Because of the rebind::other idiom, you don't have to write Registered<Registry<Registered<Registry ... and avoid the concrete circular dependency. Because the default Registry<int> is never actually used except to supply rebind, it's not instantiated and so doesn't report an error that you can't construct an int using new int ( name ).

You can then use the types for your B and A:

typedef Registered<>  B;
typedef Registry<B>   A;

int main () {
    B& b1 = A::CreateInstance("one"); // create a B

    b1.checkCondition(); // uses A to create object internally

    B b2("two"); // compile error - can only create B using A

    return 0;
}

You can of course construct a Registered< MyMockRegistry > for testing, the other main objection to circularly dependent types.

Pete Kirkham