Is this class properly encapsulated?

No, absolutely not.

Try this code with that header:

#include "screenmatrix.hpp"
#include <iostream>
int main() {
    std::cout << WIDTH << std::endl;
    return 0;
}

Those constants you're putting in there... yeah, those end up as globals.

Where on earth should they go? In the cpp.

And as far as TermChar goes, that probably just needs a forward declaration and can also be defined properly in the cpp.

Then you have a bunch of unsigned short parameters floating around. I'd strongly consider typedeffing that, eg, typedef unsigned short size. Realistically this is just plain awful anyways, since numbers like 0 - that is, unadorned literals - are int. 0U is an unsigned int, 0L is a long, and 0UL is an unsigned long. There's no suffix you can add to mean short or unsigned short so all of these parameters must come from int conversions. Just use int...

Also, your signatures suck. Take this for example:

TermChar getElement(unsigned short x, unsigned short y);

Since it returns a copy of a TermChar, is it really necessary to make this function callable only from non-const objects? Shouldn't this function be const? If it is const, then probably you want this signature:

const TermChar& getElement(unsigned short x, unsigned short y) const;

Now you don't have a copy and people can't edit the thing that's returned anyways. If someone needs a copy the client code would simply need to store it in a TermChar, eg, TermChar x = myobject.getElement(0,0); does make the copy. The difference is people can now do const TermChar& x = myobject.getElement(0,0); and it won't invoke the creation of any TermChar. In the signature you give, the above code does work - because temporaries can be bound to const references, but make no mistake about it, the code still makes a new TermChar when your signature is as you show it and there's no calling convention to make that go away.

Similarly, with this:

void setString(unsigned short x, unsigned short y, std::string s);

Unless your function plans on modifying s here, that should be a const std::string& so that the function doesn't need to make a new copy of the parameter passed into s. Since this is a set function, I don't expect const here nor a const overload as it doesn't make sense. However, with set functions I generally try to return something, namely the object itself. Meaning, I'd change this to the following:

 ScreenMatrix& setString(unsigned short x, unsigned short y, const std::string& s);

How do we actually return that? You may've seen it if you ever wrote your own operator= function. The function would end with return *this; to accomplish the return value. In this way, that function can be chained, eg, myobject.setString(0,0,"Panda").setString(0,1,"Apple");. With your signature, this must be two statements. With my signature, it can be either one or two statements, depending on what the client is doing.

/r/cpp_questions Thread