0
\$\begingroup\$

In my project I send messages, so there are items that should be sent and that were already sent. I want to have container to manage these two entities.

The container I wrote seems to work ok. It supposed to be thread unsafe, data racing issues should be managed by class users.

What might be done better? All ideas are welcome, I will be happy to know your opinions and objections.

WaitSentMap.h

#pragma once

#include <deque>

#include "MessageException.h"

template <typename T>
class WaitSentMap
{
public:
    WaitSentMap() {};
    ~WaitSentMap() {};

    template <typename U>
    void addItem( U && item );

    T& getNextItem();

    template <typename U>
    void rescheduleItem( U && item );

    template <typename U>
    void removeItem( U && item );

private:
    std::deque< std::unique_ptr<T> >        m_waitingRequests;
    std::deque< std::unique_ptr<T> >        m_sentRequests;
};

template <typename T>
template <typename U>
void WaitSentMap<T>::addItem( U && item )
{
    m_waitingRequests.push_back( std::make_unique<T>( std::forward<T>( item ) ) );
}

template <typename T>
T& WaitSentMap<T>::getNextItem()
{
    std::unique_ptr<T> item;

    if ( m_waitingRequests.empty() )
    {
        THROW_MESSAGE << "Waiting queue is empty";
    }

    try
    {
        item = std::move( m_waitingRequests.front() );
        m_waitingRequests.pop_front();

        T * pReturn = item.get();

        m_sentRequests.push_back( std::move( item ) );

        return *pReturn;
    }
    catch ( ... )
    {
        // In case of exception raised and item might be lost between two queues 
        // let's check and restore the starting state

        auto itWait = std::find( m_waitingRequests.begin(), m_waitingRequests.end(), item );
        auto itSent = std::find( m_sentRequests.begin(), m_sentRequests.end(), item );

        if( ( itSent == m_sentRequests.end() ) && ( itWait == m_waitingRequests.end() ) )
        {
            m_waitingRequests.push_front( std::move( item ) );
        }
        throw;
    }
}

template <typename T>
template <typename U>
void WaitSentMap<T>::rescheduleItem( U && item )
{
    auto it = std::find_if( m_sentRequests.begin(), m_sentRequests.end(), 
                                [&] ( std::unique_ptr<T> & p ) { return *(p.get()) == item; } );

    if ( it == m_sentRequests.end() )
    {
        THROW_MESSAGE << "Item " << item << " was not scheduled";
    }

    std::unique_ptr<T> pItem;
    try
    {
        pItem = std::move( *it );
        m_sentRequests.erase( it );
        m_waitingRequests.push_back( std::move( pItem ) );
    }
    catch ( ... )
    {
        // In case of exception raised and item might be lost between two queues 
        // let's check and restore the starting state


        auto itWait = std::find_if( m_waitingRequests.begin(), m_waitingRequests.end(),
                                        [&] ( std::unique_ptr<T> & p ) { return *(p.get()) == item; } );

        auto itSent = std::find_if( m_sentRequests.begin(), m_sentRequests.end(), 
                                        [&] ( std::unique_ptr<T> & p ) { return *(p.get()) == item; } );


        if ( ( itWait == m_waitingRequests.end() ) && ( itSent == m_sentRequests.end() ) && ( *pItem ) )
        {
            m_sentRequests.push_back( std::move( pItem ) );
        }

        throw;
    }
}

template <typename T>
template <typename U>
void WaitSentMap<T>::removeItem( U && item )
{
    auto itWait = std::find_if( m_waitingRequests.begin(), m_waitingRequests.end(),
                                    [&] ( std::unique_ptr<T> & p ) { return *(p.get()) == item; } );
    if ( itWait != m_waitingRequests.end() )
    {
        m_waitingRequests.erase( itWait );
    }

    auto itSent = std::find_if( m_sentRequests.begin(), m_sentRequests.end(), 
                                    [&] ( std::unique_ptr<T> & p ) { return *(p.get()) == item; } );

    if ( itSent != m_sentRequests.end() )
    {
        m_sentRequests.erase( itSent );
    }
}
\$\endgroup\$
3
\$\begingroup\$

Code

  • Missing a #include <memory> for std::unique_ptr, #include <algorithm> for std::find_if.
  • Use = default; or = delete; for constructors, rather than declaring an empty function. This makes the intent clearer. Don't forget the copy constructor, move constructor, copy assignment, move assignment (again, use = default or = delete where appropriate). e.g.

    WaitSentMap() = default;
    
    WaitSentMap(WaitSentMap&&) = delete;
    WaitSentMap(WaitSentMap const&) = delete;
    
    WaitSentMap& operator=(WaitSentMap&&) = delete;
    WaitSentMap& operator=(WaitSentMap const&) = delete;
    

    (I'd personally leave out the destructor definition unless it's actually needed).

  • addItem does std::forward<T> when it should be doing std::forward<U>.

  • addItem forwards a single item to the T constructor, which restricts T to types with single argument constructors. Use a template parameter pack to allow Ts with constructors with any number of arguments.

  • getNextItem throws if there are no waiting requests. However, there is no way to check if there are requests waiting, forcing the user to track this externally!

  • getNextItem attempts to recover from an internal throw. However, combined with the above, this makes the class unusable after such a recovery.

  • getNextItem: the only thing that can really throw here is push_back, so we can change the order of things to make it safe without worrying about recovery:

    m_sentRequests.emplace_back(std::move(m_waitingRequests.front()));
    m_waitingRequests.pop_front();
    
    return *m_sentRequests.back();
    
  • rescheduleItem: has the same issues, and a similar solution:

    m_waitingRequests.emplace_back(std::move(*it));
    m_sentRequests.erase(it);
    
  • Note that the find_if predicate can be simplified, and use a const&, and be defined once per function:

    auto pred = [&] (std::unique_ptr<T> const& p) { return *p == item; };
    
  • Both rescheduleItem and removeItem should take a const&, not a &&. Also, these should probably both be T, rather than U.

  • removeItem: does not return any indication of success or failure (throw or return a boolean). Again, since the user can't check what's in the queues, this makes the class rather unhelpful.


Design

  • containers: It looks like a queue would be more appropriate than a deque for m_waitingRequests, since items are pushed and processed in FIFO order.
  • containers: A vector would probably be more appropriate for m_sentRequests, as we only need to push items onto one end.

  • naming: Since all the functions manipulate items, there's not a lot of point in having the function names all end in Item.

  • naming: getNextItem is a slightly misleading name since the function also moves the item from one queue to another.

  • interface: As noted above, we need to be able to check if an item is present in each queue, or check if a queue is empty to use the class properly. I'd expect a more complete interface to look something like this:

    void push(T&& item); // was addItem
    void remove(T const& item); // was removeItem - note this should throw if the item isn't there (we now have ways to check if it's there before calling remove...)
    
    bool isWaitingEmpty() const; // waiting queue is empty
    bool isSentEmpty() const; // sent queue is empty
    bool isEmpty() const; // return isWaitingEmpty() && isSentEmpty();
    
    void clearWaiting(); // clear waiting queue
    void clearSent(); // clear sent queue
    void clear(); // clear both queues
    
    bool isWaiting(T const& item) const; // is this item in the waiting queue?
    bool isSent(T const& item) const; // is this item in the sent queue?
    bool isWaitingOrSent(T const& item) const; // is this item in either queue?
    
    T& moveNextToSent(); // was getNextItem 
    void moveToWaiting(T const& item); // was rescheduleItem
    

    Perhaps remove functions could be added for each individual queue too.


  • I think the main issue with this class is that "sending" (whatever that is) is done externally to the class itself. As such, if "sending" an item fails, the user has to then notify the container to move it back again, which is very awkward. This also seems to be the cause of using std::unique_ptr internally (because we give some external code a & to the item from getNextItem.

    If messages are sent synchronously and immediately, it might be reasonable to replace getNextItem with a function that takes a lambda to do the actual sending. Something like:

    bool send(std::function<bool(T const&)> sendMessage)
    {
        if (m_waitingRequests.empty())
            return false;
    
        if (!sendMessage(m_waitingRequests.front())) // if it can't fail, we don't need to check or return a bool from send
            return false;
    
        m_sentRequests.emplace_back(std::move(m_waitingRequests.front()));
        m_waitingRequests.pop_front();
    
        return true;
    }
    

    Alternatively, it might be more appropriate to do something based on std::future. It's hard to say without more context.

\$\endgroup\$
  • \$\begingroup\$ That's against the rules of commenting, but ... Great thanks for every point you wrote! \$\endgroup\$ – Yura Oct 7 '18 at 7:44

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.