Verify my implementation of move operations

[Christopher J. Pisz]

Can someone verify I implemented the C++11 move constructor and move
assignment operators correctly in a few classes?

I just updated
https://github.com/ChristopherPisz/ECommerce/tree/master/Domain

If someone could look at Customer and Order classes there (they are
pretty simplistic)

I am still getting used to using C++11 since past jobs didn't use modern
compilers. I think I am getting the hang of it, but want to make sure.


Copying here for the newsgroup purists:

#pragma once

// Project Includes
#include "DomainLibrary.h"

// Boost Includes
#include <boost/optional.hpp>

// Standard Includes
#include <string>

//------------------------------------------------------------------------------
class DOMAIN_API Customer
{
public:

Customer(const unsigned int Id);

Customer(const unsigned int id
, const std::string & name
, const std::string & address
, const boost::optional<unsigned int> & age);

Customer(const Customer & rhs);
Customer(const Customer && rhs);

Customer & operator = (const Customer & rhs);
Customer & operator = (const Customer && rhs);

~Customer();

unsigned int GetId() const;

std::string GetName() const;
void SetName(const std::string & name);

std::string GetAddress() const;
void SetAddress(const std::string & address);

boost::optional<unsigned int> GetAge() const;
void SetAge(boost::optional<unsigned int> age);

protected:

unsigned int m_id; // Required
std::string m_name;
std::string m_address;
boost::optional<unsigned int> m_age; // Not set = DBNULL
};



// Project Includes
#include "Customer.h"

//------------------------------------------------------------------------------
Customer::Customer(const unsigned int id)
:
m_id(id)
{
}

//------------------------------------------------------------------------------
Customer::Customer(const unsigned int id
, const std::string & name
, const std::string & address
, const boost::optional<unsigned int> & age)
:
m_id (id)
, m_name (name)
, m_address(address)
, m_age ()
{
}

//------------------------------------------------------------------------------
Customer::Customer(const Customer & rhs)
:
m_id (rhs.m_id)
, m_name (rhs.m_name)
, m_address(rhs.m_address)
, m_age (rhs.m_age)
{
}

//------------------------------------------------------------------------------
Customer::Customer(const Customer && rhs)
:
m_id (rhs.m_id)
, m_name (rhs.m_name)
, m_address(rhs.m_address)
, m_age (std::move(rhs.m_age))
{
}

//------------------------------------------------------------------------------
Customer & Customer::operator = (const Customer & rhs)
{
if( this == &rhs )
{
return *this;
}

m_id = rhs.m_id;
m_name = rhs.m_name;
m_address = rhs.m_address;
m_age = rhs.m_age;

return *this;
}

//------------------------------------------------------------------------------
Customer & Customer::operator = (const Customer && rhs)
{
if( this == &rhs )
{
return *this;
}

m_id = rhs.m_id;
m_name = rhs.m_name;
m_address = rhs.m_address;
m_age = std::move(rhs.m_age);

return *this;
}

//------------------------------------------------------------------------------
Customer::~Customer()
{
}

//------------------------------------------------------------------------------
unsigned int Customer::GetId() const
{
return m_id;
}

//------------------------------------------------------------------------------
std::string Customer::GetName() const
{
return m_name;
}

//------------------------------------------------------------------------------
void Customer::SetName(const std::string & name)
{
m_name = name;
}

//------------------------------------------------------------------------------
std::string Customer::GetAddress() const
{
return m_address;
}

//------------------------------------------------------------------------------
void Customer::SetAddress(const std::string & address)
{
m_address = address;
}

//------------------------------------------------------------------------------
boost::optional<unsigned int> Customer::GetAge() const
{
return m_age;
}

//------------------------------------------------------------------------------
void Customer::SetAge(boost::optional<unsigned int> age)
{
m_age = age;
}

#pragma once

// Project Includes
#include "DomainLibrary.h"
#include "OrderItem.h"

// Standard Includes
#include <string>
#include <unordered_map>
#include <vector>

//------------------------------------------------------------------------------
class DOMAIN_API Order
{
public:

// Unordered Map is probably the best choice since we will be
removing by ID
//
// Key - Inventory Item ID
// Value - The OrderItem
typedef std::unordered_map<unsigned int, OrderItem> CollectionType;

enum OrderStatus
{
CREATED = 0
, ORDERED // We got paid
, READY_FOR_DELIVERY
, DELIVERED
, CANCELED
};

Order(const unsigned int id
, const unsigned int customerId);

Order(const unsigned int id
, const unsigned int customerId
, const CollectionType & orderItems);

Order(const Order & rhs);
Order(const Order && rhs);

Order & operator = (const Order & rhs);
Order & operator = (const Order && rhs);

virtual ~Order();

unsigned int GetId() const;
unsigned int GetCustomerId() const;

OrderStatus GetOrderStatus() const;
void SetOrderStatus(const OrderStatus orderStatus);

void AddOrderItem(const OrderItem & orderItem);
void RemoveOrderItem(unsigned int itemId);

CollectionType GetOrderItems() const;
void SetOrderItems(const CollectionType & orderItems);

protected:

unsigned int m_id;
unsigned int m_customerId;

OrderStatus m_status;
CollectionType m_orderItems;
};


// Project Includes
#include "Order.h"

// Standard Includes
#include <algorithm>
#include <iterator>

//------------------------------------------------------------------------------
Order::Order(const unsigned int id
, const unsigned int customerId)
:
m_id (id)
, m_customerId(customerId)
, m_status (CREATED)
{
}

//------------------------------------------------------------------------------
Order::Order(const unsigned int id
, const unsigned int customerId
, const CollectionType & orderItems)
:
m_id (id)
, m_customerId (customerId)
, m_status (CREATED)
, m_orderItems (orderItems)
{
}

//------------------------------------------------------------------------------
Order::Order(const Order & rhs)
:
m_id (rhs.m_id)
, m_customerId (rhs.m_customerId)
, m_status (rhs.m_status)
, m_orderItems (rhs.m_orderItems)
{
}

//------------------------------------------------------------------------------
Order::Order(const Order && rhs)
:
m_id (rhs.m_id)
, m_customerId(rhs.m_customerId)
, m_status (rhs.m_status)
{
std::move(rhs.m_orderItems.begin(), rhs.m_orderItems.end(),
std::inserter(m_orderItems, m_orderItems.begin()));
}

//------------------------------------------------------------------------------
Order & Order::operator = (const Order & rhs)
{
if( this == &rhs )
{
return *this;
}

m_id = rhs.m_id;
m_customerId = rhs.m_customerId;
m_status = rhs.m_status;
m_orderItems = rhs.m_orderItems;

return *this;
}

//------------------------------------------------------------------------------
Order & Order::operator = (const Order && rhs)
{
if( this == &rhs )
{
return *this;
}

m_id = rhs.m_id;
m_customerId = rhs.m_customerId;
m_status = rhs.m_status;
std::move(rhs.m_orderItems.begin(), rhs.m_orderItems.end(),
std::inserter(m_orderItems, m_orderItems.begin()));

return *this;
}

//------------------------------------------------------------------------------
Order::~Order()
{
}

//------------------------------------------------------------------------------
unsigned int Order::GetId() const
{
return m_id;
}

//------------------------------------------------------------------------------
unsigned int Order::GetCustomerId() const
{
return m_customerId;
}

//------------------------------------------------------------------------------
Order::OrderStatus Order::GetOrderStatus() const
{
return m_status;
}

//------------------------------------------------------------------------------
void Order::SetOrderStatus(const OrderStatus orderStatus)
{
m_status = orderStatus;
}

//------------------------------------------------------------------------------
void Order::AddOrderItem(const OrderItem & orderItem)
{
m_orderItems.insert(std::make_pair(orderItem.GetInventoryItemId(),
orderItem));
}

//------------------------------------------------------------------------------
void Order::RemoveOrderItem(unsigned int itemId)
{
m_orderItems.erase(itemId);
}

//------------------------------------------------------------------------------
Order::CollectionType Order::GetOrderItems() const
{
return m_orderItems;
}

//------------------------------------------------------------------------------
void Order::SetOrderItems(const CollectionType & orderItems)
{
m_orderItems = orderItems;
}

//------------------------------------------------------------------------------

[Manfred]

On 3/14/2017 4:56 AM, Christopher J. Pisz wrote:
> Can someone verify I implemented the C++11 move constructor and move
> assignment operators correctly in a few classes?
>

I didn't check the details, but one thing that I saw is that move
assignment and constructors typically have non-const rhs arguments (they
may modify the rhs as they 'move' the value out of it)
Note that the standard allows for a X(const X&&) move constructor
(12.8.1 as it also allows for a X(X&) copy constructor) although its
example marks it as "OK, but possibly not sensible".

>
> Customer(const Customer & rhs);
> Customer(const Customer && rhs);

Customer(Customer && rhs);

>
> Customer & operator = (const Customer & rhs);
> Customer & operator = (const Customer && rhs);

Customer & operator = (Customer && rhs);
>

[Alf P. Steinbach]

On 14-Mar-17 4:56 AM, Christopher J. Pisz wrote:
>
>[snip]

> Copying here for the newsgroup purists:
>

[code snipped]

I tried a few times, some years ago, just attaching the code files, and
apparently that worked both technically and psychologically for all readers.

In Thunderbird those text file attachments appear as directly readable
text at the bottom of the message.

I am not sure, however, how well it works with archiving services, so I
think it's best to restrict it to supplementary information, e.g. “I've
attached the complete files so that readers can try this out”.


Cheers!,

- Alf

[Scott Lurndal]

"Alf P. Steinbach" <alf.p.stein...@gmail.com> writes:
>On 14-Mar-17 4:56 AM, Christopher J. Pisz wrote:
>>
>>[snip]
>> Copying here for the newsgroup purists:
>>
>[code snipped]
>
>I tried a few times, some years ago, just attaching the code files, and
>apparently that worked both technically and psychologically for all readers.
>

That's a broad statement, to be sure.

In fact, the NNTP client that I use doesn't handle attachments gracefully.

[Paavo Helde]

On 14.03.2017 5:56, Christopher J. Pisz wrote:
> Can someone verify I implemented the C++11 move constructor and move
> assignment operators correctly in a few classes?
>
>

> Customer::Customer(const Customer && rhs)
> :
> m_id (rhs.m_id)
> , m_name (rhs.m_name)
> , m_address(rhs.m_address)
> , m_age (std::move(rhs.m_age))

This does not call the boost::optional move constructor, because of the
'const'. Instead, it calls the standard copy constructor, which is
probably not what you intended.

Normally a user-defined move constructor should look like this:

Customer::Customer(Customer && rhs)
:
m_id (rhs.m_id)
, m_name (std::move(rhs.m_name))
, m_address(std::move(rhs.m_address))
, m_age (std::move(rhs.m_age))
{}

Ditto for other rvalue thingies.

Also, for such classes you do not actually need to write your own move
constructors or assignments at all. If you delete all your copy/move
constructors/assignments plus the destructor the compiler will generate
all this stuff by itself.