Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The following three source files is to define and test a class StackLayer.

While it was written in a need for scalable layer-based architecture design, it was also a practice for std::shared_ptr (I'm new to C++11).

I'm not sure if I'm understanding the concept and best practices for std::weak_ptr and std::shared_ptr.

You will notice that the LayerNode struct is completely public to derived classes, which I couldn't avoid because of some error std::shared_ptr was complaining about ("conversion exists but inaccessible"). I don't like this because it ruins all the encapsulation.

I'm also not sure if I have any memory leak or not (at least it is not breaking).

To prevent lower layer to access higher layer, I also had to make GetLayer function return null if that case happens. Should I assert or throw on this case instead of returning nullptr?

I only tested it in Visual C++ CPT120 (might have missing 'typename' errors in gcc, or not if I get lucky)

Singleton.h

template<typename Derived>
class Singleton
{
public:
    // Getter
    static Derived& GetInstance( void )
    {
        static Derived s_instance;
        return s_instance;
    }
protected:
    // Ctor
    Singleton( void ){}
    // Dtor
    virtual ~Singleton( void ){}
private:
    // NO COPY
    Singleton( const Singleton& rhs );
    // NO ASSIGNMNET
    Singleton& operator=( const Singleton& rhs );
}; // class Singleton

LayerStack.h

#ifndef LAYER_STACK_INCLUDE_GUARD
#define LAYER_STACK_INCLUDE_GUARD
#include "Singleton.h"      // Singleton
#include <vector>           // std::vector
#include <memory>           // std::shared_ptr std::weak_ptr std::dynamic_casting
#include <unordered_map>    // std::unordered_map
#include <cassert>          // assert
#include <type_traits>      // std::is_pointer
#include <algorithm>        // std::for_each
#include <functional>       // std::function

class LayerStack;

// public Inherit this class to make a layer.
// layer is to be added to an instance of LayerStack before calling GetLayer function.(Look at LayerStack::Push function)
template <typename Derived>
class Layer : public LayerStack::LayerNode, public Singleton<Derived>
{
    friend class LayerStack;

public:
    typedef Derived instance_type;
    Layer( void )
        : LayerNode( typeid(Derived).hash_code() )
        , Singleton()
    {
    }
    template <typename T>
    std::shared_ptr<T> GetLayer( void )
    {
        return LayerNode::m_pLayerStack->QueryLayerFrom<T>(m_attitue);
    } // std::shared_ptr<T> GetLayer( void )
    virtual ~Layer( void ) {}
}; // class Layer : public LayerStack::LayerNode, public Singleton<Derived>

// Layer Stack.
// Can store instance of layer inheriting class Layer.
// These rules are to be followred:
//      * Each type of layer can only appear once in the same layer stack.( Layer is Singleton. Look at Layer definition )
//      * Layer can access other layer by calling GetLayer function, but not in ascending order( Only layer pushed eairler than caller layer can be accessed. In other words, top layer can acess all layers, while bottom layer cannot access any layer)
class LayerStack
{
    template <typename Derived>
    friend class Layer;
private:
    typedef std::size_t type_hash_type;
    typedef std::size_t size_type;
    struct LayerNode
    {
        LayerNode( type_hash_type typeHash )
            : m_pLayerStack(nullptr)
            , m_typeHash(typeHash)
            , m_attitue(0)
        {
        }
        virtual ~LayerNode( void ) {}
        LayerStack*                 m_pLayerStack;
        LayerStack::type_hash_type  m_typeHash;
        int                         m_attitue;
    };
public:
    typedef std::unordered_map<type_hash_type, std::weak_ptr<LayerNode>> layer_map_type;
    typedef std::vector<std::shared_ptr<LayerNode>> layer_vec_type;
private:
    layer_map_type m_layerMap;
    layer_vec_type m_layerVec;
public:
    LayerStack( void )
        : m_layerMap()
        , m_layerVec()
    {
    }
    template <typename T>
    void Push( std::shared_ptr<T>&& item )
    {
        static_assert(!std::is_pointer<T>::value, "Cannot use a shared pointer to a pointer");
        // Gets type hash code
        type_hash_type h = typeid(T).hash_code();
        // Search item by the hash code from the map.
        // item shouldn't be duplicate(Singleton)
        assert(m_layerMap.find(h) == m_layerMap.end());
        // Set Parent pointer
        item->m_pLayerStack = this;
        // Add item to the vector
        m_layerVec.push_back(item);
        // Add item to the map
        m_layerMap[h] = item;
        // Set attitue
        item->m_attitue = m_layerVec.size();
    } // void Push( std::shared_ptr<T>&& item )
    void Pop( void )
    {
        m_layerMap.erase(m_layerVec.back()->m_typeHash);
        m_layerVec.pop_back();
    } // void Pop( void )
    template <typename CAST_TYPE>
    void ForAscendingEach( std::function<void(CAST_TYPE*)> callback)
    {
        std::for_each( m_layerVec.begin(), m_layerVec.end(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            callback(std::dynamic_pointer_cast<CAST_TYPE>(spNode).get());
        });
    }
    template <typename CAST_TYPE>
    void ForDescendingEach( std::function<void(CAST_TYPE*)> callback)
    {
        std::for_each( m_layerVec.rbegin(), m_layerVec.rend(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            callback(std::dynamic_pointer_cast<CAST_TYPE>(spNode).get());
        });
    }
    template <typename CAST_TYPE, typename ...Args>
    void ForAscendingEach( void (CAST_TYPE::*callback)(Args...) , Args... args)
    {
        std::for_each( m_layerVec.begin(), m_layerVec.end(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            (std::dynamic_pointer_cast<CAST_TYPE>(spNode).get()->*callback)(args...);
        });
    }
    template <typename CAST_TYPE, typename ...Args>
    void ForDescendingEach( void (CAST_TYPE::*callback)(Args...) , Args... args)
    {
        std::for_each( m_layerVec.rbegin(), m_layerVec.rend(), [&]( std::shared_ptr<LayerNode> spNode )
        {
            (std::dynamic_pointer_cast<CAST_TYPE>(spNode).get()->*callback)(args...);
        });
    }
    size_type Size( void )
    {
        return m_layerVec.size();
    } // size_type Size( void )
    template <typename T>
    std::shared_ptr<T> QueryLayer( void )
    {
        assert( m_layerMap.find(typeid(T).hash_code()) != m_layerMap.end() );

        return std::dynamic_pointer_cast<T>(m_layerMap[typeid(T).hash_code()].lock());
    }
private:
    template <typename T>
    std::shared_ptr<T> QueryLayerFrom( int attitue )
    {
        assert( m_layerMap.find(typeid(T).hash_code()) != m_layerMap.end() );

        return m_layerMap[typeid(T).hash_code()].lock()->m_attitue < attitue
            ? std::dynamic_pointer_cast<T>(m_layerMap[typeid(T).hash_code()].lock())
            : nullptr; 
    }
}; // class LayerStack

Main.cpp

#include "LayerStack.h"
#include <iostream>

#define PROMPT(MSG) do { std::cout << " * " << (MSG) << std::endl; } while(0)
#define SHOW(EXPR) do { std::cout << (#EXPR) << " : " << (EXPR) << std::endl; } while(0)

class Base
{
public:
    virtual ~Base( void ){}
    virtual void Print( void ) = 0;
    virtual void PrintInt( int i ) = 0;
};

class A : public Layer<A>, public Base
{
public:
    virtual void Print( void ) override
    {
        std::cout << "A Printing" << std::endl;
    }
    virtual void PrintInt( int i ) override
    {
        std::cout << "A Printing " << i << std::endl;
    }
};

class B : public Layer<B>, public Base
{
public:
    virtual void Print( void ) override
    {
        std::cout << "B Printing" << std::endl;
    }
    virtual void PrintInt( int i ) override
    {
        std::cout << "B Printing " << i << std::endl;
    }
};

void PrintBase( Base* pBase )
{
    pBase->Print();
}

int main( void )
{
    LayerStack stack;

    PROMPT("Pushing A");
    stack.Push( std::make_shared<A>() );
    SHOW(stack.Size());

    PROMPT("Pushing B");
    stack.Push( std::make_shared<B>() );
    SHOW(stack.Size());

    PROMPT("Now Stack Looks like:");
    PROMPT("  [Top] B");
    PROMPT("  [Bottom] A");

    PROMPT("Query A Directly from the stack layer");
    std::shared_ptr<A> sptra;
    SHOW(sptra = stack.QueryLayer<A>());

    PROMPT("Query B Directly from the stack layer");
    std::shared_ptr<B> sptrb;
    SHOW(sptrb = stack.QueryLayer<B>());

    PROMPT("Call function in ascending order");
    stack.ForAscendingEach<Base>(PrintBase);

    PROMPT("Call function in descending order");
    stack.ForDescendingEach<Base>(PrintBase);

    PROMPT("Call member function in ascending order");
    stack.ForAscendingEach<Base>(&Base::PrintInt, 1);

    PROMPT("Call member function in descending order");
    stack.ForDescendingEach<Base>(&Base::PrintInt, 2);

    PROMPT("Accessing A from B should success(A is lower than B.(A is pushed eairlier) descending access is allowed)");
    SHOW(sptra = sptrb->GetLayer<A>());

    PROMPT("Accessing B from A should fail(B is higher than A.(B is pushed later) ascending access is prevented.)");
    SHOW(sptrb = sptra->GetLayer<B>());

    PROMPT("Popping A");
    stack.Pop();
    SHOW(stack.Size());

    PROMPT("Popping B");
    stack.Pop();
    SHOW(stack.Size());

    return 0;
} // int main( void )

Output:

 * Pushing A
stack.Size() : 1
 * Pushing B
stack.Size() : 2
 * Now Stack Looks like:
 *   [Top] B
 *   [Bottom] A
 * Query A Directly from the stack layer
sptra = stack.QueryLayer<A>() : 01009074
 * Query B Directly from the stack layer
sptrb = stack.QueryLayer<B>() : 0100D544
 * Call function in ascending order
A Printing
B Printing
 * Call function in descending order
B Printing
A Printing
 * Call member function in ascending order
A Printing 1
B Printing 1
 * Call member function in descending order
B Printing 2
A Printing 2
 * Accessing A from B should success(A is lower than B.(A is pushed eairlier) de
scending access is allowed)
sptra = sptrb->GetLayer<A>() : 01009074
 * Accessing B from A should fail(B is higher than A.(B is pushed later) ascendi
ng access is prevented.)
sptrb = sptra->GetLayer<B>() : 00000000
 * Popping A
stack.Size() : 1
 * Popping B
stack.Size() : 0
share|improve this question
2  
Just a quick comment; you have declared your Singleton copy and copy-assignment operators as private and not implemented them to prevent copying; but the C++11 way to do this would be to write: Singleton( const Singleton& ) = delete; –  Shaktal Aug 3 at 19:23
    
Please remember to vote on any answers you've found helpful, now that you have sufficient rep. Code Review is always in need of votes. –  Jamal Aug 4 at 0:01

2 Answers 2

up vote 4 down vote accepted

Some minor things:

  • Include guard names don't need an _INCLUDE_GUARD at the end. They should not be confused with macros anyway, although you won't have very many of them in C++.

  • Consider organizing your library #includes, such as alphabetically. This will make it easier to keep track of all of them.

  • Comments like //Ctor and //Dtor are noise since it's already obvious. Comments shouldn't point out the obvious; they should provide useful information.

  • Since your types are named in PascalCase, name your functions in camelCase or snake_case. It's generally good to use separate naming types for these two things.

  • All the std::endls can be replaced with "\n". You don't need to flush the buffer so many times (which is what the former also does), and the latter will just output a newline.

  • You don't need an explicit return 0 at the end of main(). Successful termination is already implied at this point, so the compiler will do this return for you here.

  • You don't need a comments after a closing brace, especially at the end of main(). It may be okay if you have many nested ones, but it's just clutter for others.

  • Unlike in C, a void parameter is not needed for functions taking no arguments.

share|improve this answer
    
I thought that it was considered bad practice (if not undefined behavior) to assume an implicit return value from main(). Also, std::endl is a better idea than "\n" because it will handle line endings correctly on the unfortunate systems that use "\r\n" instead. I'd also recommend #includes being grouped by system, then library, then local, then sorted within each group. –  fluffy Aug 3 at 21:31
    
@fluffy: Maybe in C, but this is a special case. If something can fail (such as opening a file), then main() should return something else. I don't see "\r\n" coming up too often, but in most cases, std::endl isn't particularly needed. As for the #includes, I was referring to the libraries specifically, since more of them are used here. But you're right that different #include types should be in a certain order. –  Jamal Aug 3 at 21:37
    
You don't see \r\n coming up too often? You realize that Windows still has the largest desktop OS install base in the world, right? Not to mention that most textual network protocols use \r\n for their line endings... –  fluffy Aug 3 at 21:43
4  
@fluffy 1) The C++ standard guarantees that zero would be returned by default for the main function if you leave that out. 2) Completely false. Even on windows systems, std::endl appends a '\n'. The compiler is responsible for sorting out the line endings on its own. 3) Bad advice once again. Local headers, then libraries, and then platform-specific headers. Each group should then be sorted alphabetically. –  jliv902 Aug 3 at 21:47
    
@jliv902 Okay, thanks for the clarification on 1 and 2. However, 3 seems to just be a matter of taste. Can you back that up with any specific reasoning for why it should be local-first? –  fluffy Aug 3 at 21:51

I have a few things to add to what @Jamal already mentioned:

  • If you want a class to be uncopyable, don't make its copy constructor and assignment operator private. Instead, mark them as deleted functions:

    Singleton( const Singleton& rhs ) = delete;
    Singleton& operator=( const Singleton& rhs ) = delete;
    

    The = delete is explicit by itself, so I guess that you can also remove the comments about the class not being copyable.

  • I don't think that you will use Singleton as a base class for polymorphism (you won't create Singleton* instances to store derived class instances), so you don't have to make it destructor virtual. Generally speaking, virtual destructors are useful when you need to delete a instance via a base class pointer.

  • I don't think that you need to specify what you use in each included header. While it might be useful for <memory> or <algorithm>, what you will use in <vector>, <unordered_map> and <cassert> is totally obvious.

  • It might be a good idea to forget the old typedefs and use the brand new type aliases instead. Their advantages are multiple: the syntax is closer to a variable declaration and to a namespace alias, and it can be templated to create an alias template. Using type aliases will help you to write consistent code:

    using instance_type = Derived;
    
  • This kind of "closing comment" is not really useful:

    } // class Layer : public LayerStack::LayerNode, public Singleton<Derived>
    

    A good indentation will provide all the information you need. Moreover, IDEs and code editors generally highlights matching braces, further reducing the need for this kind of comments. You shouldn't write too many comments that are somehow obvious or uneeded, for after a modification of the code, a comment may lie and a lying comment is worse than no comments at all.

  • When a method does not modify the enclosing class, you should const-qualify it:

    size_type Size( void ) const
    {
        return m_layerVec.size();
    }
    

    There are many functions in your code that do not modify their classes and that ought to be const-qualified.

share|improve this answer
    
1 Had no idea delete could be sued like that, thank you. 2 Indeed virtual destructor there was meaningless.. In fact, I just realized that Singleton class is not doing anything in this hierarchy... 3 This one is true, but I like to be consist. –  user2883715 Aug 3 at 23:34
    
4 Didn't know using alias could replace typedef, thanks. 5 I do think this is useful if source is too long. Especially when I'm not looking at the code from IDE 6 I was almost forgetting about const qualification. Nice to have them. –  user2883715 Aug 3 at 23:40

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.