Fixing bad code

[EccentricDuck]

So I spent about 2 days going through working with 3D primitives in XNA using BasicEffect which is their pre-defined shader 1.1 class (and I've only scratched the surface - haven't even really gotten into buffers, lighting, etc.) and trying to get a basic triangle with colored points to draw (red, green, and blue). I wound up spending several hours (and a couple hours flipping to other things since I wasn't getting anywhere) going through my code to figure out why my implementation didn't work when the tutorial example that I was following did (they were almost exactly the same except that I made a separate class for mine since I intend on doing some work with primitives in a project that I'm working on).

Turns out that it wasn't anything complex, it was just the way I created the vertices:

Code: Select all

            vertices            = new VertexPositionColor[3];
            vertices[0]         = new VertexPositionColor(new Vector3(0, 1, 0), Color.Red);
            vertices[0]         = new VertexPositionColor(new Vector3(0.5f, 0, 0), Color.Green);
            vertices[0]         = new VertexPositionColor(new Vector3(-0.5f, 0, 0), Color.Blue);

I was tired at the time of originally creating it and created (and overwrote) the same vertice 3 times... and for that I spent hours manually debugging my code looking for errors that wouldn't have been picked up by the compiler. Any tips for catching errors like this... or stories you'd like to share about similar experiences?

[K-Bal]

The wrong indices are not that worse. Your allocation is wrong:

Code: Select all

vertices = new VertexPositionColor[3];

Okay, now we have three objects of type VertexPositionColor.

Code: Select all

vertices[0] = new VertexPositionColor

Unneccessary, it's already allocated.

Code: Select all

new Vector3(0, 1, 0)

You can create such a small data type on the stack.

[Bakkon]

^ Yep. You'll wanna do something like this accessing the properties instead of creating new instances.

Code: Select all

vertices = new VertexPositionColor[3]; 
vertices[0].Position = new Vector3(-0.5f, -0.5f, 0f);
vertices[0].Color = Color.Red;
vertices[1].Position = new Vector3(0, 0.5f, 0f);
vertices[1].Color = Color.Green;
vertices[2].Position = new Vector3(0.5f, -0.5f, 0f);
vertices[2].Color = Color.Yellow;

[K-Bal]

I don't even see why to allocate a Vector3.

[Bakkon]

This is C# with XNA.

[K-Bal]

This is C# with XNA.

Touché

[EccentricDuck]

I don't even see why to allocate a Vector3.

Either way I'm assigning a Vector3 value to the "position" field in Vertices[x] using an instance of Vector3 (that's deleted as soon as the method finishes since it doesn't have a pointer). I see what you're saying with creating new instances of each vertice though. In that case I was just following the code that he wrote and taking advantage of the class' constructor for assigning those values.

My main problem lied in the fact that I used vertices[0] 3 times rather than once and using vertices[1] and vertices[2] which is what I'm using now - and I completely missed that every time I went over my code. I was thinking that my issue might have to do with a non-updated instance of their GraphicsDevice class since I passed an instance to the primitive class, though testing proved that to be false.

Does creating a new instance of each vertice use much extra space on the heap (keeping in mind that C# uses quite an efficient garbage collector and gets rid of an unreferenced instance as soon as the method finishes)?

[EccentricDuck]

I am actually really wondering whether I should adopt more of the approach that you gave (individual field declarations rather than using a constructor where you need to pass the values first). Does this make a big difference to efficiency in larger applications? If so then I'd like to program the way that you suggested as a matter of habit. If not then I'd like to continue using constructors as I have been because they provide a way of making more efficient use of my time.

Thanks

Edit: Hey, one more thought. I know it's supposed to be "good coding practice" to not make all your fields and methods public (of course I understand that "good coding practice" doesn't always mean best way or most practical), and wouldn't accessing each individual field require making all those fields public? By using a constructor call I can keep most fields private. Just something that came to mind.

[EccentricDuck]

At the risk of being a little obnoxious I'm going to bump this thread. I have a lot of respect for you guys as programmers and I'd really like to know which way you'd program and why. I listed why I program the way I do - extensive use of constructors for efficiency of time, use of private members, and (perhaps less mentioned) to keep the necessary members bundled together in an easy to reference location.

[Falco Girgis]

There is absolutely nothing wrong with your code. Doing what you did:

Code: Select all

         vertices            = new VertexPositionColor[3];
            vertices[0]         = new VertexPositionColor(new Vector3(0, 1, 0), Color.Red);
            vertices[1]         = new VertexPositionColor(new Vector3(0.5f, 0, 0), Color.Green);
            vertices[2]         = new VertexPositionColor(new Vector3(-0.5f, 0, 0), Color.Blue);

Is equivalent to:

Code: Select all

vertices = new VertexPositionColor[3];
vertices[0].Position = new Vector3(-0.5f, -0.5f, 0f);
vertices[0].Color = Color.Red;
vertices[1].Position = new Vector3(0, 0.5f, 0f);
vertices[1].Color = Color.Green;
vertices[2].Position = new Vector3(0.5f, -0.5f, 0f);
vertices[2].Color = Color.Yellow;

The only difference is the fact that you are allowing your constructor to initialize your fields up on the VertexPositionColor's construction. The constructor method is not going to be any slower.

I listed why I program the way I do - extensive use of constructors for efficiency of time, use of private members, and (perhaps less mentioned) to keep the necessary members bundled together in an easy to reference location.

Good for you. There's absolutely nothing wrong with that. It's considered good OO design.

[..]and wouldn't accessing each individual field require making all those fields public? By using a constructor call I can keep most fields private. Just something that came to mind.

Oh, most definitely. That is considered good design. In this particular case (VertexPositionColor) there is no real reason for the XNA framework to hide (or "encapsulate") the Color or Position values. I'm pretty sure they are actually implemented as "properties" (according to the official C# style guide, the capitalized field names are denoting properties).

Traditionally (when I say traditionally, I refer to C++), member variables (is what we call 'em) are private when their change has an effect on something else. Imagine a class where changing the value of a variable has an effect on something else. When I change something like the shape of a RigidBody class, that change means that I have to recalculate the center of mass, the mass, moment of inertia, and a few other things. Now instead of making the programmer do it manually (be responsible for updating other variables when one changes) like in the days of C, this functionality is "encapsulated" or hidden from the user.

In C#, we are seeing a little bit of a change. With properties we don't really see get/set member functions ("methods" in C#) any more--instead we see "properties" which allow us to encapsulate this logic and allow our end-user to access our fields stupidly as if they were mere variables.

In the case of this particular scenario (VertexPositionColor), we have a very trivial little class where there is no real reason to hide any of the variables from the user. As a matter of fact, the "class" is probably actually a "struct" for this reason--a value type, rather than a reference type. This means that it's stored on the stack rather than the heap. It is recommended that you use that approach for trivial classes that require no real encapsulation and exposes all fields. Since it is stored on the stack, its allocation is also faster than reference types. It will also be deleted or "popped off the stack" the minute the function (errr... I mean "method") finishes execution without the CLR having to run the garbage collector on it.

[K-Bal]

There is absolutely nothing wrong with your code. Doing what you did:

Code: Select all

         vertices            = new VertexPositionColor[3];
            vertices[0]         = new VertexPositionColor(new Vector3(0, 1, 0), Color.Red);
            vertices[1]         = new VertexPositionColor(new Vector3(0.5f, 0, 0), Color.Green);
            vertices[2]         = new VertexPositionColor(new Vector3(-0.5f, 0, 0), Color.Blue);

Well, I know a shit about C# so if anyone could clear up that matter.

Coming from a C++ background I'd assume that all three objects are already allocated after the first line.

[Falco Girgis]

There is absolutely nothing wrong with your code. Doing what you did:

Code: Select all

         vertices            = new VertexPositionColor[3];
            vertices[0]         = new VertexPositionColor(new Vector3(0, 1, 0), Color.Red);
            vertices[1]         = new VertexPositionColor(new Vector3(0.5f, 0, 0), Color.Green);
            vertices[2]         = new VertexPositionColor(new Vector3(-0.5f, 0, 0), Color.Blue);

Well, I know a shit about C# so if anyone could clear up that matter.

Coming from a C++ background I'd assume that all three objects are already allocated after the first line.

OH MY GOD, DUDE! I WAS THINKING THAT SAME THING!

Yeah, a C++ developer looks at that, says that you're allocating 3 vectors, then setting each index equal to a new vector, causing you to end up with 6 total (3 unused). That's not the case in C#.

Arrays have to be thought of as a list of REFERENCES, not values. When you do:

Code: Select all

vertices = new VertexPosition[3];

You are actually allocating an array of size 3. Each index in the array is a reference to a VertexPosition (which is set to NULL).

So now you have to initialize the indices to reference a VertexPosition:

Code: Select all

            vertices[0]         = new VertexPositionColor(new Vector3(0, 1, 0), Color.Red);
            vertices[1]         = new VertexPositionColor(new Vector3(0.5f, 0, 0), Color.Green);
            vertices[2]         = new VertexPositionColor(new Vector3(-0.5f, 0, 0), Color.Blue);

It still makes me shit my brain. JIT takes some getting used to from a C++ background.

[K-Bal]

Thank you for clearing that up.

[EccentricDuck]

Thanks, that clears a lot up. I didn't get to this sooner since I took a few days away from the forums to get some other stuff done. Hehe, I definitely think in terms of JIT. My C++ experience was very limited (I barely got into using arrays and my understanding of the "under the hood" stuff was pretty minimal, hence why I didn't get where the comments about already declaring the array were coming from). I assume I'll be going back that way this fall though with a couple comp sci courses (though it may start with Python... which is what I picked up and used to learn the majority of my intro to the basics - bet I'd see it a lot differently than I did in December when I last used it).

Glad to see a critique of properties too. I don't really use them since, a) if it's a private member then it's that way because I don't need to reference it externally, b) if I need to access it I'll make it protected, public, or whatever depending upon what I need it for if I want external access, c) properties take more time and space where it seems unnecessary - why would I want to reiterate the fields that I just declared with capital letters to now make them, essentially, public and take at least 8 lines to do so. I can see times to use them where you'd do more than what I just said (like a Sum property where you get x1, x2, and return their sum), but the vast majority of examples I've seen essentially use them to make private fields public.