What can I improve in this code

Started by
15 comments, last by DiegoSLTS 9 years, 5 months ago

It's plenty good actually - I guess that's what I was looking for, thanks.

There's an issue with this though:

class Branch {
std::vector<Branch*> childrens;

~Branch() {
for (//iterate over childrens) {
delete childBranch;
}
}

// everything else
}

Well I guess it would work with a recursive function:

void destroyChildren()
{
for (auto iter = children.begin(); iter != children.end(); iter++)
{
childBranch->destroyChildren();
delete childBranch;
}
return;
}

I hope I didn't think up some bs recursive function, cause I'm scared to test it in my code.

Though I guess now is the moment to ask - what works better(faster) - going through your structure with a recursive function, or having things stored in a vector and using brute force to solve the issues (for example I had a variant of the same program that did everything with loops rather than recursive functions (though it did pretty much the same now that I think about it))?

You don't need that recursion to destroy objects, it's recursive by default if you use the destructor. When you iterate over the childrens and call "delete childBranch", the destructor of each branch is called, which iterates over it's childrens and so on. You shouldn't have a method to destroy things, unless you need that functionality separated from the desctruction of the object itself. If you wanted to clear all the Tree you could have a "clear" method that destroys everything and keeps the "Tree" instance alive, but for cleaning purposes when exiting the program just call the destructors.

You are right. Till now whenever I was facing such situations I was always wondering whether to hardcode it or to do a check even if I know what I am going to get. I'd try to go for readability over needless optimizations from now on.

Always! tongue.png

I actually wanted to dodge this issue by implementing branches.push_back() in the ctor of branches, but for that reason it would need to see the branches vector which is in Tree. And I've actually faced the problems with deleting you are talking about the previous time I coded something like this (it was not in c++ though ,and had a graphical visualization), I had to manage a few storage class instances, and I must say I was accessing a lot of null pointers, just because I forgot to remove an element here or there.

Practice recursion a little more, Trees are like the most common case where recursive functions are actually good, sometimes in college they make you write a recursive function for stuffs that could be easily done with a for loop, but working with trees gets a lot simpler with recursion.

On a side note (I'm not really sure whether I should make a new thread for this) speaking of nested classes and classes design. I cannot think of a way to code this without a friend declaration:


//Let's say we have a class Engine that can draw and load our meshes
//However we shouldn't be able to create a mesh - that should be delegated to
//the engine's function loadMesh, We should be able to do something like this.
Engine myEngine;
Mesh* myMesh = myEngine.loadMesh( "some_filename.3ds" );
myMesh.translate(1,3,5);
myMesh.rotate( 30, 45, 0 );

//however this should be illegal:
Mesh* mesh2 = new Mesh();

And here's what I think up of as a construction:


// ... includes
#include <vector>

class Mesh
{
	friend class Engine;

private:
	float transformationMatrix[4][4];
	ID3D11Buffer* vertexBuffer;
	ID3D11Buffer* indexBuffer;

public:
	void translate(const float vec[3]);
	void rotate(const float vec[3]);

private:
	Mesh();
	Mesh(const Mesh&);
	~Mesh();
};

class Engine
{
private:
	std::vector<Mesh*> meshes;
	//... stuff
public:
	//... stuff
	void loadMesh(std::string filename)
	{
		Mesh* temp = new Mesh;
		meshes.push_back(temp);
	}

	void drawMesh()
	{
		//draw the mesh by using the vertex and index buffers
		//and the transformation matrix
	}

	~Engine()
	{
		//...free stuff
		for (auto iter = meshes.begin(); iter != meshes.end(); iter++)
		{
			delete *iter;
		}
		meshes.clear();
	}
};

Any way to do it without friend declarations?

Do you really need to prevent a Mesh being created manually? What's the problem with that? Most graphics libraries let you load a mesh or create it (directly or as a derived class).

Anyway, I'm not sure how to do that (never had the need) but as a first attempt you could make Mesh an abstract class, so "new Mesh();" wouldn't work, and have an inner class in Engine extending Mesh with the missing methods. It doesn't completelly hide the constructors tought, any class deriver from Mesh could be a concrete instantiable class just like the one inside Engine.

Advertisement

Any way to do it without friend declarations?

Make the constructor/destructor public. You can still declare copy constructors, etc., as private.

If you do so, you can also have a bool Mesh::Init(...params...) function. That allows the engine to check for out-of-memory conditions, and let the Mesh class validate other (private) initializations.

E.g.,


Mesh* temp = new Mesh;
if( nullptr == temp ) { .. handle out-of-memory conditions ...}
if( !temp->Init(..params..) )
{
   // handle bad init/load errors
   delete temp;
   return false; // or other condition indication
}



EDIT: A benefit of that approach is to reuse code. If Mesh doesn't need to know about Engine, you can use Mesh in another application.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

You don't need that recursion to destroy objects, it's recursive by default if you use the destructor.

Idk how I didn't realize that laugh.png

About the Mesh, Engine:

I just didn't want to have a mesh with non initialized vertex and index buffers, so I wanted to to abstract that from the user + I want to keep track of the meshes allocated on the heap internally in the Engine class's meshes vector. Still even if I say that it's over-engineering to try and hide the ctor and dtor, and even if I make them public, then I still cannot access the vertexBuffer and indexBuffer and the transformationMatrix from Engine with the drawMesh method(when drawing a mesh you need to access the ib,vb and the transformation matrix). And I really want the vb,ib and the matrix to be private... Maybe I should go for a non OOP design idk. Otherwise I can only think of using friend(maybe it's unavoidable)...


I would return a signed int, and "-1" if the item isn't there.

It's also fine to return -1 as an unsigned int to indicate an error condition.

The value of -1 cast to an unsigned int is 0xFFFFFFFF, which is an easy error condition to test for, and highly unlikely to be a valid array index in a 32-bit program.

You'll find this pattern all over the std library, in the form of static_cast<size_t>(-1).

Yea, I do this alot.

One thing to note though, is that if you are accidentally mixing two different variable types, it can lead to bugs later if, for example, you migrate from 32 bit to 64 bit, and one of the types turns 64 bit, and the other does not. Like in the standard library, with static_cast<size_t>(-1).

Example:


unsigned pos = str.find("blah");

//*Never* true on 64bit machines, because 'pos' is 32bit and 'std::string::npos' is 64 bit, so 32bitInteger can never equal 64bitInteger(-1)
if(pos == std::string::npos)
{
    //...
}

So I guess the real more warning here is don't mix and match types. smile.png


I really want the vb,ib and the matrix to be private

Uh.. what exactly do you want Mesh to be? An object with no constructor, no destructor, no access to data it holds... blink.png

If you add Mesh::DrawYourself(device*, context*, resources,...) then you've got a new rendering engine. Not good.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

I just wanted pretty much everything for the mesh to be handled by the engine, with he exception of the rotation, translation etc. methods. I never mentioned the mesh should draw itself, even in my code I put drawMesh in the Engine class. I just don't want people to be able to mess up the matrix/vertexBuffer or indexBuffer manually. I want everything from creating the mesh to destroying it to be delegated as methods to the engine - basically I want pretty much every operation that is done on the mesh to pass through the engine - quite the opposite to the mesh drawing itself. But maybe that is bad design idk, to me at least it made sense that the engine should serve to load/draw/keep track and destroy it's resources. So that is basically my question - how do I make it (with best design practices in mind) so that the Engine at least draws the mesh without me having to make the mesh's private variables public?

Uh.. what exactly do you want Mesh to be?

It has a ctor and dtor - they're just private, check the sample code up there it explains what exactly I want the mesh to be(basically you should be able to create a mesh only by doing Engine::loadMesh() ):


//Let's say we have a class Engine that can draw and load our meshes
//However we shouldn't be able to create a mesh - that should be delegated to
//the engine's function loadMesh, We should be able to do something like this.
Engine myEngine;
Mesh* myMesh = myEngine.loadMesh( "some_filename.3ds" );
myMesh.translate(1,3,5);
myMesh.rotate( 30, 45, 0 );

//however this should be illegal:
Mesh* mesh2 = new Mesh();

I don't even emphasize on the fact that the ctor and dtor need to be private, I can make them public. The main issue is that I cannot make the matrix/vertex and index buffers public.

P.S. I am just having OOP practices in mind in this scenario - things irrelevant to the user should be abstracted from him - so I try to hide the more I can. I mean why would you create a mesh with no geometry information? And the other thing is that I want to keep track of the allocated memory for the meshes on the heap so I can deallocate the memory when the time comes.

You can try something like this:

Have a class MeshTransformation that keeps track of every transformation.

On the Engine define Mesh inside Engine class so Mesh is not accessible. Put a MeshTransformation attribute in Mesh.

When you load a Mesh in the engine return a pointer to the MeshTransformations attribute of that Mesh instead of the Mesh itself.

When drawing the Mesh use the MeshTransformation data, it would be updated by the user.

Now the user can create a MeshTransformation object that doesn't change anything by it's own, because it would not be associated with any Mesh inside the engine.

I guess this would work for what you want. I don't think you can do more than this, but if creating an instance of a usless object is also something you want to prevent you should go back to friend classes.

EDIT:

"P.S. I am just having OOP practices in mind in this scenario - things irrelevant to the user should be abstracted from him - so I try to hide the more I can. I mean why would you create a mesh with no geometry information?"

If it doesn't make sense in your code application, that's OK, but someone might want to edit a mesh in more detail, not just transforming it. Other people might want to create the mesh dynamically, so there's no file to load (a 3D terraing generated with a height map, or a shape that needs to follow a bezier curve only known at run time, there are plenty of uses). In those cases creating a "blank" mesh and adding vertices might be a requirement.

This topic is closed to new replies.

Advertisement