[RESOLVED] Basic operator overloading

[Gerald Bates]

I have just finished adding basic operator overloaded functionality in my vmSize class and need to ask 3 questions.

A. Have I implemented this functionality correctly?
B. Are there any unforseen issues that I am not aware of?

C. With respect to Set/Get methods; Is it acceptable to implicitly inline these methods within the class, or should I remove the implementatation into the .cpp file and place the keyword inline in front of them instead?

// vmSize.h

#ifndef __VMSIZE_H__
#define __VMSIZE_H__

namespace vmStd {

class vmSize {
public:
   vmSize();
   vmSize(int width, int height);
   ~vmSize();

   const vmSize &operator=(const vmSize &sz);
   bool operator==(const vmSize &sz) const;
   bool operator!=(const vmSize &sz) const;
   const vmSize operator+(const vmSize &sz) const;
   const vmSize operator-(const vmSize &sz) const;
   const vmSize &operator+=(const vmSize &sz);
   const vmSize &operator-=(const vmSize &sz);

   void SetWidth(int width) { m_nWidth = width; }
   void SetHeight(int height) { m_nHeight = height; }
   void SetSize(int width, int height) { m_nWidth = width; m_nHeight = height; }
   int  GetWidth() const { return m_nWidth; }
   int  GetHeight() const { return m_nHeight; }

private:
   int m_nWidth, m_nHeight;
};

} // namespace vmStd

#endif // __VMSIZE_H__

// vmSize.cpp

#include "vmSize.h"

using namespace vmStd;

vmSize::vmSize() : m_nWidth(0), m_nHeight(0) {}

vmSize::vmSize(int width, int height) : m_nWidth(width), m_nHeight(height) {}

vmSize::~vmSize() {}

const vmSize &vmSize::operator=(const vmSize &sz)
{
   if(&sz != this)
      this->SetSize(sz.GetWidth(), sz.GetHeight());

   return *this;
}

bool vmSize::operator==(const vmSize &sz) const
{
   if(this->GetWidth() == sz.GetWidth() && this->GetHeight() == sz.GetHeight())
      return true;

   return false;
}

bool vmSize::operator!=(const vmSize &sz) const
{
   return !(*this == sz);
}

const vmSize vmSize::operator+(const vmSize &sz) const
{
   vmSize temp;
   temp.SetSize(this->GetWidth() + sz.GetWidth(),
                this->GetHeight() + sz.GetHeight());

   return temp;
}

const vmSize vmSize::operator-(const vmSize &sz) const
{
   vmSize temp;
   temp.SetSize(this->GetWidth() - sz.GetWidth(),
                this->GetHeight() - sz.GetHeight());

   return temp;
}

const vmSize &vmSize::operator+=(const vmSize &sz)
{
   this->SetSize(this->GetWidth() + sz.GetWidth(),
                 this->GetHeight() + sz.GetHeight());

   return *this;
}

const vmSize &vmSize::operator-=(const vmSize &sz)
{
   this->SetSize(this->GetWidth() - sz.GetWidth(),
                 this->GetHeight() - sz.GetHeight());

   return *this;
}

[monarch_dodra]

The usual standard practice is to make binary operators non-member fields. In this case it does no change much (if at all), just know this exists.

Also, from a style point of view, in C++ the de-facto style guide says "*"/"&" is next to the *type*, not the variable. EG:
"vmSize& sz" vs "vmSize &sz"

The exception is "C-like" C++, or ported "from C" code, where those are next to the variable name. There's actually a reason for this, but a bit long to explain.

const vmSize operator+(const vmSize &sz) const;

Why does this return a const?

const vmSize &operator+=(const vmSize &sz);

Why does *this* return const? This makes it impossible to chain the operators, "(a += b) += c;"

void SetWidth(int width);

It can be convenient for these setter functions to return a reference to this. This allows for a builder pattern, such as:
myVm.setWidth(5).setHeight(5);

In this case, the user can use setSize(), yes. But again, know this is something users might expect.

const vmSize vmSize::operator+(const vmSize &sz) const
{
   vmSize temp;
   temp.SetSize(this->GetWidth() + sz.GetWidth(),
                this->GetHeight() + sz.GetHeight());

   return temp;
}

You usually implement operatorX in terms of "operator+X". It's much easier:

const vmSize vmSize::operator+(const vmSize &sz) const
{
   return vmSize(*this) += sz;
}

Notice how this code works for all operators. Don't duplicate your code.

Other than that, I see nothing outright WRONG with your code. The *only* thing that I'm not really sure about whether you should be having these at all: Does it make sense to add sizes? It depends on what your vmSize represents:
- If it's the size of a rectangle, I would say no.
- If it defines a (mathematical) vector, then I would say yes.

One last thing: When you need to overload such operators, consider using boost:perators. You only provie "==", "+=", "-=", and then it does all the boilerplate required for the rest.
http://www.boost.org/doc/libs/1_57_0.../operators.htm

Like this:

#include <iostream>
#include <boost/operators.hpp>

class vmSize
    : boost::equality_comparable<vmSize
    , boost::additive<vmSize> >
{
public:
   vmSize();
   vmSize(int width, int height);
   ~vmSize();

   vmSize &operator=(const vmSize &sz);
   bool operator==(const vmSize &sz) const;
   vmSize &operator+=(const vmSize &sz);
   vmSize &operator-=(const vmSize &sz);

   void SetWidth(int width) { m_nWidth = width; }
   void SetHeight(int height) { m_nHeight = height; }
   void SetSize(int width, int height) { m_nWidth = width; m_nHeight = height; }
   int  GetWidth() const { return m_nWidth; }
   int  GetHeight() const { return m_nHeight; }

private:
   int m_nWidth, m_nHeight;
};

vmSize::vmSize() : m_nWidth(0), m_nHeight(0) {}

vmSize::vmSize(int width, int height) : m_nWidth(width), m_nHeight(height) {}

vmSize::~vmSize() {}

vmSize &vmSize::operator=(const vmSize &sz)
{
   if(&sz != this)
      this->SetSize(sz.GetWidth(), sz.GetHeight());

   return *this;
}

bool vmSize::operator==(const vmSize &sz) const
{
   if(this->GetWidth() == sz.GetWidth() && this->GetHeight() == sz.GetHeight())
      return true;

   return false;
}

vmSize &vmSize::operator+=(const vmSize &sz)
{
   this->SetSize(this->GetWidth() + sz.GetWidth(),
                 this->GetHeight() + sz.GetHeight());

   return *this;
}

vmSize &vmSize::operator-=(const vmSize &sz)
{
   this->SetSize(this->GetWidth() - sz.GetWidth(),
                 this->GetHeight() - sz.GetHeight());

   return *this;
}

int main()
{
   vmSize a(2, 3);
   vmSize b(2, 3);
   vmSize c = a + b;
   std::cout << c.GetWidth() << " " << c.GetHeight() << std::endl;
}

[laserlight]

Also, from a style point of view, in C++ the de-facto style guide says "*"/"&" is next to the *type*, not the variable. EG:
"vmSize& sz" vs "vmSize &sz"

The exception is "C-like" C++, or ported "from C" code, where those are next to the variable name. There's actually a reason for this, but a bit long to explain.

Thankfully, Stroustrup has an explanation ready: Is ``int* p;'' right or is ``int *p;'' right?

By the way, I noticed that you are using __VMSIZE_H__ as the header inclusion guard identifier. Identifiers beginning with underscores followed by an uppercase letter, or that contain consecutive underscores, are reserved to the implementation for any use, so you should not use them. I used to dismiss this as a purely theoretical concern, until the time when we encountered a person's whose bug was really because he/she did not adhere to this rule. So, improbable as a conflict may be, use an identifier that is not reserved (and use a convention that will make it highly likely to be unique).

[Gerald Bates]

Many thanks for the reply. I have been working through all these points raised and changing/testing code. I am still trying to implement myVm.setWidth(5).setHeight(5); functionality and have a few errors. This is a new subject to me and I've only had a book as a reference in learning this topic. I've got a lot to learn. Will post a proper reply once finished.

[monarch_dodra]

Many thanks for the reply. I have been working through all these points raised and changing/testing code. I am still trying to implement myVm.setWidth(5).setHeight(5); functionality and have a few errors.

This should be pretty trivial. Just make them return a reference "vmSize&", and at the end of your function, return a reference to this "return *this". That's really all there is to it:

vmSize& SetHeight(int height) { m_nHeight = height; return *this; }

This is a new subject to me and I've only had a book as a reference in learning this topic. I've got a lot to learn. Will post a proper reply once finished.

For what it's worth, you are off to a good start.

Also, you might want to consider implementing an "operator<<" for printing.
https://msdn.microsoft.com/en-us/library/1z2f6c2k.aspx

In your case, maybe print something like "(2,3)".

[Gerald Bates]

The usual standard practice is to make binary operators non-member fields. In this case it does no change much (if at all), just know this exists.

Not sure what this means?

Also, from a style point of view, in C++ the de-facto style guide says "*"/"&" is next to the *type*, not the variable. EG:
"vmSize& sz" vs "vmSize &sz"

I have changed the left side and within parenthesis where I use vmSize&. I assume you mean in all places.

The issue with returning const was because I was confused. I thought if you returned a pointer then you could assign it to another and thus change private data of a class that had gone out of scope. I don't believe that is an issue here, I have removed const completely and hopefully that is correct.

It can be convenient for these setter functions to return a reference to this. This allows for a builder pattern, such as:
myVm.setWidth(5).setHeight(5);

Completed this. I actually managed to implement SetWidth() and SetHeight() before I read your reply, thanks.

You usually implement operatorX in terms of "operator+X". It's much easier:

My book talked on this but I am finding this idiom difficult to master. Hopefully with practice and more experience I will get better. I did however change operator- after seeing your example. I don't think I can improve on operator+= or operator-=, but I could be wrong?

As for the inclusion guard - I have removed underscores at the beginning and the end and tried to use a unique name.

Operator<< will be done later.

Boost is something I have heard of, and will indeed get round to looking at this.

The changed code now looks like this:

#ifndef VMAPI_SIZE_H
#define VMAPI_SIZE_H


namespace vmStd {


class vmSize {
public:
   vmSize();
   vmSize(int width, int height);
   ~vmSize();


   vmSize& operator=(const vmSize& sz);
   bool operator==(const vmSize& sz) const;
   bool operator!=(const vmSize& sz) const;
   vmSize operator+(const vmSize& sz) const;
   vmSize operator-(const vmSize& sz) const;
   vmSize& operator+=(const vmSize& sz);
   vmSize& operator-=(const vmSize& sz);


   vmSize& SetWidth(int width) { m_nWidth = width; return *this; }
   vmSize& SetHeight(int height) { m_nHeight = height; return *this; }
   void SetSize(int width, int height) { m_nWidth = width; m_nHeight = height; }
   int  GetWidth() const { return m_nWidth; }
   int  GetHeight() const { return m_nHeight; }


private:
   int m_nWidth, m_nHeight;
};


} // namespace vmStd


#endif // VMAPI_SIZE_H

#include "vmSize.h"


using namespace vmStd;

vmSize::vmSize() : m_nWidth(0), m_nHeight(0) {}
vmSize::vmSize(int width, int height) : m_nWidth(width), m_nHeight(height) {}
vmSize::~vmSize() {}

vmSize& vmSize::operator=(const vmSize& sz)
{
   if(&sz != this)
      this->SetSize(sz.GetWidth(), sz.GetHeight());


   return *this;
}

bool vmSize::operator==(const vmSize& sz) const
{
   if(this->GetWidth() == sz.GetWidth() && this->GetHeight() == sz.GetHeight())
      return true;


   return false;
}

bool vmSize::operator!=(const vmSize& sz) const
{
   return !(*this == sz);
}

vmSize vmSize::operator+(const vmSize& sz) const
{
   return vmSize (*this) += sz;
}

vmSize vmSize::operator-(const vmSize& sz) const
{
   return vmSize (*this) -= sz;
}

vmSize& vmSize::operator+=(const vmSize& sz)
{
   this->SetSize(this->GetWidth() + sz.GetWidth(),
                 this->GetHeight() + sz.GetHeight());


   return *this;
}

vmSize& vmSize::operator-=(const vmSize& sz)
{
   this->SetSize(this->GetWidth() - sz.GetWidth(),
                 this->GetHeight() - sz.GetHeight());


   return *this;
}

[laserlight]

My book talked on this but I am finding this idiom difficult to master. Hopefully with practice and more experience I will get better. I did however change operator- after seeing your example. I don't think I can improve on operator+= or operator-=, but I could be wrong?

Your implementation of operator+= looks fine to me, though I would not bother prefixing the member function calls for the current object with this->.

One thing to think about is the notion of simple interfaces. You can start by reading an interview of Bjarne Stroustrup, the designer and original implementer of C++, on his thoughts about designing simple interfaces. For further reading:
How Non-Member Functions Improve Encapsulation by Scott Meyers
GotW #84: Monoliths "Unstrung" by Herb Sutter

For your vmSize class, what this means is that instead of having operator+ as a member function like what you wrote here:

vmSize vmSize::operator+(const vmSize& sz) const
{
   return vmSize (*this) += sz;
}

You could have it as a non-member non-friend function:

vmSize operator+(vmSize lhs, const vmSize& rhs)
{
   return lhs += rhs;
}

In fact, since your operator+= only uses public member functions of the vmSize class, you could make it a non-member non-friend function too:

vmSize& operator+=(vmSize& lhs, const vmSize& rhs)
{
   lhs.SetSize(lhs.GetWidth() + rhs.GetWidth(),
               lhs.GetHeight() + rhs.GetHeight());

   return lhs;
}

Likewise operator==, operator!=, operator-= and operator- can be non-member non-friend functions.

[Gerald Bates]

I have read some of the supplement material and have come to the conclusion that some of my functionality should not be members but implemented as non-member non-friend functions. I have two classes which are almost identical but perform a different task. What strikes me is that both classes are designed to have the same operator overloading functionality, I am duplicating code where the only difference with the code is the class type I am dealing with?

Is it possible to have two classes, vmSize and vmPoint that are generic and have ONLY the basic functionality and share the operator overloaded functionality between them? I could implement a new file called vmOperator which could be included only if I needed that functionality. As I understand it, this would reduce duplicated code and improve encapsulation.

Is it possible to achieve this?

[2kaud]

where the only difference with the code is the class type I am dealing with

I think you are referring to templates. See
http://www.codeproject.com/Articles/...mplates-Part-1

There is also derived classes. See
https://msdn.microsoft.com/en-us/library/a48h1tew.aspx

[monarch_dodra]

For operator=, checking for that this and the right hand side are the same is usually a good idea. But if you can write your code in such a way that it does not matter, that's equally good. Also, for your operator==, you don't need the "if". just return the boolean condition directly:

vmSize& vmSize::operator=(const vmSize& sz)
{
   this->SetSize(sz.GetWidth(), sz.GetHeight());
}

bool vmSize::operator==(const vmSize& sz) const
{
   return this->GetWidth() == sz.GetWidth() && this->GetHeight() == sz.GetHeight()
}

[monarch_dodra]

One thing to think about is the notion of simple interfaces. You can start by reading an interview of Bjarne Stroustrup, the designer and original implementer of C++, on his thoughts about designing simple interfaces. For further reading:
How Non-Member Functions Improve Encapsulation by Scott Meyers
GotW #84: Monoliths "Unstrung" by Herb Sutter

[...]

In fact, since your operator+= only uses public member functions of the vmSize class, you could make it a non-member non-friend function too:

These are good reading, but (IMO), both fail to recognize that having (certain) functions as members is simply convenient from a user's point of view. Just because you can, doesn't always mean you should. IMO, there is no reason to have "operator X=" as non-member. At best, it pollutes your namespace.

As a matter of fact, the standard disallows making "operator=" non-member, when it is functionally no different from +=.

IMO.

[laserlight]

IMO, there is no reason to have "operator X=" as non-member.

As I observed earlier, the given operator+= only uses public member functions of the vmSize class, hence it does not access the internals, thus there is no reason (as observed) to have it as a member.

If say there is concern that those public member functions might do extra checking where none is required here, then it would make sense for operator+= to be a member instead so as to have direct access to the internals.

At best, it pollutes your namespace.

That concern is not relevant here since it is a legitimate part of the interface associated with the namespace; one could just as well say that the class name pollutes the namespace in which it is in.

As a matter of fact, the standard disallows making "operator=" non-member, when it is functionally no different from +=.

I believe that has to do with the fact that the compiler would provide, as a member function, an implicit copy assignment operator unless the code says otherwise.

[razzle]

I have just finished adding basic operator overloaded functionality in my vmSize class and need to ask 3 questions.

I'm using the slightly different "friend" approach. I've found it to be very simple, safe and straightforward. Not all operators can be friends but most can. I'm using it for small primitive-like classes, for example the 3D Index in the example below.

The class is copyable and immutable (there are no modfying operators (such as +=) and no setter methods (just getters) so objects never change once created). This lends a nice functional touch to it in my view.

This example is a work in progress and I add operators on a "need to have" basis (including operators that cannot be friends but must be class members). Operators are kept basic and not allowed to grow or be overly complex (in which case they are made free functions).

		class Index {
		public:
			Index(int x, int y, int z) : k(x), j(y), i(z) {}

			int x() const {return k;}
			int y() const {return j;}
			int z() const {return i;}

			friend Index operator+(const Index& l, const Index& r) {
				return {l.k+r.k, l.j+r.j, l.i+r.i};
			}
			friend Index operator+(const Index& l, int c) {
				return {l.k+c, l.j+c, l.i+c};
			}
			friend Index operator-(const Index& l, const Index& r) {
				return {l.k-r.k, l.j-r.j, l.i-r.i};
			}
			friend bool operator==(const Index& l, const Index& r) {
				return l.k==r.k && l.j==r.j && l.i==r.i;
			}
			friend bool operator!=(const Index& l, const Index& r) {
				return l.k!=r.k || l.j!=r.j || l.i!=r.i;
			}
			friend bool operator>=(const Index& l, const Index& r) {
				return l.k>=r.k && l.j>=r.j && l.i>=r.i;
			}
			friend bool operator>=(const Index& l, int c) {
				return l.k>=c && l.j>=c && l.i>=c;
			}
			friend bool operator<(const Index& l, const Index& r) {
				return l.k<r.k && l.j<r.j && l.i<r.i;
			}
			friend bool operator<(const Index& l, int c) {
				return l.k<c && l.j<c && l.i<c;
			}
			friend Index operator*(const Index& l, int c) {
				return {l.k*c, l.j*c, l.i*c};
			}
			friend Index operator*(int c, const Index& r) {
				return r*c;
			}
			friend Index operator/(const Index& l, int c) {
				return {l.k/c, l.j/c, l.i/c};
			}
			friend Index operator<<(const Index& l, int n) {
				return {l.k<<n,l.j<<n,l.i<<n};
			}
			friend Index operator>>(const Index& l, int n) {
				return {l.k>>n,l.j>>n,l.i>>n};
			}
			friend Index abs(const Index& l) {
				return {std::abs(l.k), std::abs(l.j), std::abs(l.i)};
			}
			friend int max(const Index& l) {
				return std::max(std::max(l.k, l.j), l.i);
			}
			friend std::string string(const Index& l) {
				return "(" + std::to_string(l.k) + "," + std::to_string(l.j) + "," + std::to_string(l.i) + ")";
			}

		private:
			int k,j,i;
		};

There's no particular reason to split the implementation into separate .h and .cpp files (there never is but that's another story ). A .h file only is fine, just make sure there's a proper include guard and that non-template free functions are declared inline.