clarification on "proper" way to handle information hiding

[Elize]

I have a class whose purpose is to store a lot of data for access by other classes. This data is stored in arrays and never altered. However, its constructor function has a lot of work to do in reading, parsing, and organizing that data. This involves creating several lists different types, each of unknown size, one item at a time, then cutting chunks of lists apart and shoving them inside of other lists. So, during the constructor function, the data is handled with linked lists. At the end of the constructor function, the data in the linked lists is pared down into arrays, and the linked lists are destroyed. Nothing outside of the class, or even outside of the constructor function, needs to access these linked lists or to know they exist at all.

So, the classes which handle these linked lists are private subclasses of the class that handles the data. Currently, they are set up to allow each other unfettered access to one another, even hypothetically in ways that could cause memory leaks, but blocked from being accessed by anything outside the parent class. My thinking is that these classes exist only as temporary tools to facilitate the completion of a single operation, so as long as I don't do anything that could cause problems in that operation, it is okay to structure them that way. I CAN set them up so that they hide their data internally even from the parent class, and are only accessible through member functions which ensure that their data is all managed properly and won't cause memory leaks. However, if I do so, the program is much less efficient, by a significant margin. I'm trying to be better about only writing "good" code, and the source code for this project will be visible after it is complete. My question is, how "improper" is the way I have it set up, from a standard C++ coding practices perspective?

EDIT: would it even be possible to create the classes inside the function itself? If so, would that be more proper?

[kbw]

What you've described so far seems fine to me.

[mbozzi]

So, the classes which handle these linked lists are private subclasses of the class that handles the data.

This sounds like a violation of the LSP, although it would be much easier to comment if there was actually code to look at.

What do you mean by private subclass?

[Elize]

I mean it's set up like:

class A {
private:

	class B {
	public:
		class BNode {
			int _bData;
			BNode* _prev;
			BNode* _next;
		};
		BNode* _first;
		BNode* _last;
		int _length;
	};

	class C {
	public:
		class CNode {
			char _cData;
			CNode* _prev;
			CNode* _next;
		};
		CNode* _first;
		CNode* _last;
		int _length;
	};

	int* _intArray;
	int _intArrayLength;
	char* _charArray;
	int _charArrayLength;
public:
	A() {
		//build and manipulate a bunch of B and Cs
		//once finished with that, then assign the organized data to the appropriate private member variables and destroy all B and C
		//never use B or C again
	}
};

[Thomas1965]

Your private subclasses are actually inner classes. A Subclass normally inherits from another class.
Is there a reason why you prefer pointers over std::vector in class A ?
Also why don't you use std::list if you have to use a list at all?
If you tell us what the app is about maybe we can find a simpler way.

[gunnerfunner]

once finished with that, then assign the organized data to the appropriate private member variables and destroy all B and C

B provides an int (_bData) and C provides a char (_cData) – so effectively this int and this char are assigned …

to the appropriate private member variables (of A)

– so why can't these be organized and passed w/o using the inner classes? As Thomas states, you'd have to provide some more, tangible information to take this forward

[dhayden]

So, the classes which handle these linked lists are private subclasses of the class that handles the data.

Make them completely invisible to users of class A. Put them inside the implementation file:
A.h:

class A {
public:
	A() {
		//build and manipulate a bunch of B and Cs
		//once finished with that, then assign the organized data to the appropriate private member variables and destroy all B and C
		//never use B or C again
	}
};

A.cpp:

class B {
	public:
		class BNode {
			int _bData;
			BNode* _prev;
			BNode* _next;
		};
		BNode* _first;
		BNode* _last;
		int _length;
	};

	class C {
	public:
		class CNode {
			char _cData;
			CNode* _prev;
			CNode* _next;
		};
		CNode* _first;
		CNode* _last;
		int _length;
	};
A::A() {
    // Do stuff with classes B and C
}

[JLBorges]

> Make them completely invisible to users of class A. Put them inside the implementation file:

To make them programatically invisible to users of class A,
they should be placed in an unnamed namespace inside the implementation file.

A.cpp

#include "a.h"

namespace // anonymous namespace
{
     struct B // since these classes are insulated (not visible ouside a.cpp),
              // we may make everything in them public
     {
         // ...
     };

     // etc.
     
}

If classes B and C are used only within the constructor of A, and the class definitions are just a few lines,
place the definitions inside the body of the constructor.

A::A()
{
     struct B // since these classes are local (not visible ousidethe constructor),
              // we may (should) make everything in them public
     {
         // ...
     };

     // etc.
     
}

[Elize]

>Is there a reason why you prefer pointers over std::vector in class A?
Yes. They don't actually store ints and chars. The program reads in 3d meshes, triangulates them, then cuts them into cubes, including adding caps internally along the cut planes. There are potentially millions of vertices being read in one at a time, as well as UV and normal data, and then indices for those items assigned to different faces. With linked lists, I can easily cut an n-gon into triangles, and I can easily cut faces into cubes, and I can easily make the cap faces that go inside the cuts, because that just involves taking some elements of one list and putting them in another list. If I use vectors, I have to rewrite entire lists to cut a face, which takes forever, and also I'm making vectors of vectors because I have a list of faces of unknown length each containing a list of vertices of unknown length, and the memory requirements become insane. Once the mesh is read in, triangulated, and cut into cubes, I can just collapse it down to an array of vertices and an array of indices to those vertices which make triangles, and I no longer use the lists.

>B provides an int (_bData) and C provides a char (_cData)
No it doesn't. The code snippet I posted is a simplification. There are more than just B and C, and they store more complicated data structures than ints and chars, and they have more functions inside of them. I'm sorry if I didn't make that clear. The actual project so far spans ~20 files and is ~4500 lines of code. If I posted enough of it to give you adequate context, no one would spend the several hours it would take to figure out what's going on in there.
It's more like B provides a point and C provides texture coordinates and D provides normals and E provides a list of Fs which provide numerical indices read in from the file, which get converted into Gs which provides a list of Hs which provide a list of pointers to a B C and D, which get converted into Is which provide exactly 3 pointers each to Bs, Cs, and Ds, and also a face normal and face area and bounding box. Then when everything is Bs, Cs, Ds, and Is, the data from the groups of BCDs get put into an array, and the pointers in the Is get converted into three indices into that array and those along with the face normal and area and bounding box are stored in another array, and all BCDEFGHI are destroyed.

>If classes B and C are used only within the constructor of A, and the class definitions are just a few lines, place the definitions inside the body of the constructor.
B and C are only used in the constructor of A, but the class definitions are not just a few lines. Again, what I posted was a simplification, and there are more than two of them. Nonetheless, this looks like the way to do it, even though it makes the constructor huge (the constructor was already huge). Thank you.

[gunnerfunner]

seldom have I read a more lucid description of a seemingly complicated problem. It seems you have a good handle on what needs to be done and you're trying to figure out the how's which is usually easier after the what's. Good luck!

[JLBorges]

> B and C are only used in the constructor of A, but the class definitions are not just a few lines.
> Again, what I posted was a simplification, and there are more than two of them.
> Nonetheless, this looks like the way to do it, even though it makes the constructor huge
> (the constructor was already huge).

There is nothing wrong with placing these (somewhat big) classes in an anonymous namespace inside the implementation file.

There is a trade off.
Is it ok for the unit of encapsulation be the whole component (a.cpp) instead of a single function (the constructor)?
Or would we rather avoid writing a huge and unwieldy constructor?

[JLBorges]

> it makes the constructor huge (the constructor was already huge).

If the construction of A is in itself a big abstraction, it would be a good idea to encapsulate that part in a separate component (say, an A_builder class).

For instance:

a_builder.h

struct A_builder
{
    A_builder( /* ... */ ) ;

    // members similar to or identical to those in A
};

a_builder.cpp

#include "abuilder.h"

namespace
{
    // implementation details
    // classes B, C, D etc.
    // ...
}

A_builder::A_builder( /* ... */ ) { /* initialise members */ }

a.h

#include "abuilder.h"

struct A
{
    A( A_builder ) ; // initialise using result of processed info in A_builder
                     // (move where appropriate)
                     
    A( /* args */ ) : A( A_builder( /* args */ ) ) {} // delegating constructor
    
    // ...
};

[dhayden]

>> Make them completely invisible to users of class A. Put them inside the implementation file:

> To make them programatically invisible to users of class A,
> they should be placed in an unnamed namespace inside the implementation file.

Can you explain how the unnamed namespace makes them less visible than putting them in the implementation file? - Thanks.

[JLBorges]

A named class declared at namespace scope has external linkage unless it is declared directly or indirectly within an unnamed namespace.

As an example, if x.cpp has the definition of a class S at global namespace scope, and unaware of this, someone else defines a different class of their own, also named S, at global namespace scope, this would be a violation of ODR.

Note: in C, there is no program-wide ODR for types, and even extern declarations of the same variable in different translation units may have different types as long as they are compatible.

In C++, the source-code tokens used in declarations of the same type must be the same as described above: if one .cpp file defines struct S { int x; }; and the other .cpp file defines struct S { int y; };, the behavior of the program that links them together is undefined.

This is usually resolved with unnamed namespaces.

http://en.cppreference.com/w/cpp/language/definition