Sorry your browser is not supported!

You are using an outdated browser that does not support modern web technologies, in order to use this site please update to a new browser.

Browsers supported include Chrome, FireFox, Safari, Opera, Internet Explorer 10+ or Microsoft Edge.

Dark GDK / Crud code (I think I'm bad at this)

Author
Message
ionstream
20
Years of Service
User Offline
Joined: 4th Jul 2004
Location: Overweb
Posted: 3rd Sep 2008 05:53
Hi everyone. Well then, for the past week and a half or so I've been trying to write a game so that I could make a tutorial on how to make a game in C++. I'm almost done I guess, I need to add collision handling for the enemies and then some more enemy logic, then I guess the main menu and saving. But I think my code is, well, crap-tastic. For a large part the code is probably bad because I avoided singletons, templates, and STL, but for the most part the overall structure would have remained the same if I used those. A large number of parts are convoluted, there are a million files for only 1500 lines of code. Any comments on the code would be greatly appreciated, as I would like to one day make this stupid game fun and educational for people learning C++.

Attachments

Login to view attachments
Skull breaker
16
Years of Service
User Offline
Joined: 29th Aug 2008
Location:
Posted: 3rd Sep 2008 06:47
if u add comments to the code it would prolly make it more legible for people to read

This is a signiture
sydbod
16
Years of Service
User Offline
Joined: 14th Jun 2008
Location: Just look at the picture
Posted: 3rd Sep 2008 15:06
Well if that is CRUD code then .... me slinks away with head hanging low
Zotoaster
19
Years of Service
User Offline
Joined: 20th Dec 2004
Location: Scotland
Posted: 3rd Sep 2008 16:35
I don't know.. Your code looks perfectly fine to me. It's formatted very nicely, very commented, and each section is well fitted within the respected files. You have lots of inheritance going on, which really shows some nice structure.

The only real problem I can find is that the game will have some memory leaks. You are allocating memory all over the place but not deleting it afterwards.

Benjamin
21
Years of Service
User Offline
Joined: 24th Nov 2002
Location: France
Posted: 3rd Sep 2008 22:31
One tip I can give you is never use dynamic memory when you don't have to. Your allocation of 'game' is a good example of where it isn't necessary.

ionstream
20
Years of Service
User Offline
Joined: 4th Jul 2004
Location: Overweb
Posted: 3rd Sep 2008 23:02
Yeah you guys are write, I really abused the "new" keyword! I'm going to try to fix that. Also yeah I should have commented it heavily before I sent it for review.

I'm going to try to switch over to STL and use maps and vectors when I can, but I have a problem with that. Currently sprites are stored in a static array, so that they are allocated only once in the entire program, at the very beginning, and all the sprites are set to active=false. When createSprite is called, it searches for the first sprite where active = false, and then sets active to true and uses that. This makes deleting a sprite really fast because all it has to do is set active to false again and it won't get rendered.
How would I go about doing that with a vector? My first thought was to create an std::vector<Sprite *> that would be simple enough to call push_back on to create a new sprite, but as far as I know vector has no "find" method, so I would have to iterate through the whole vector to find the sprite I'm trying to delete. Would that be a sufficient method, or is there a better STL type I should be using?

Thanks for your input!

Benjamin
21
Years of Service
User Offline
Joined: 24th Nov 2002
Location: France
Posted: 3rd Sep 2008 23:50
Quote: "My first thought was to create an std::vector<Sprite *>"

If you use pointers for this kind of thing you're always the one in charge of allocation and deallocation, and it leaves a lot of room for mistakes. Thus I suggest using std::vector<Sprite>.

Vectors allow you to access elements by index using the [] operator, just like with regular arrays. I'm not sure if a vector is a best thing to use though, as it means you have to manually find a free slot yourself. You'd be better off using some kind of linked list, and store pointers to links rather than storing sprite numbers. I'm not sure how one would go about implementing this with std::list though.

Zotoaster
19
Years of Service
User Offline
Joined: 20th Dec 2004
Location: Scotland
Posted: 4th Sep 2008 01:22
Quote: "If you use pointers for this kind of thing you're always the one in charge of allocation and deallocation, and it leaves a lot of room for mistakes. Thus I suggest using std::vector<Sprite>."


There's a neat garbage collection template class in the STL called auto_ptr. I've never used it personally, but it sounds pretty neat - I'll give it a try some time.



Quote: "Vectors allow you to access elements by index using the [] operator, just like with regular arrays. I'm not sure if a vector is a best thing to use though, as it means you have to manually find a free slot yourself. You'd be better off using some kind of linked list, and store pointers to links rather than storing sprite numbers. I'm not sure how one would go about implementing this with std::list though."


Not sure what you're saying about manually having to find free slots, unless you are talking about deactivating sprites and then having to replace them later with active ones, which yeah, would including some iteration.

As for using a linked list, I believe if was you Ben that told me it's very slow to move through them, heheh.

I say stick to vectors, but lose the active/inactive stuff. You can use iterators to delete elements in the vector. Of course, if you ever want to keep track of a single sprite, this would take extra effort, but I don't see where this would get too cumbersome.

Benjamin
21
Years of Service
User Offline
Joined: 24th Nov 2002
Location: France
Posted: 4th Sep 2008 01:28 Edited at: 4th Sep 2008 01:34
Quote: "There's a neat garbage collection template class in the STL called auto_ptr."

But why use a solution to something that isn't necessary in the first place?

Quote: "As for using a linked list, I believe if was you Ben that told me it's very slow to move through them, heheh."

What do you mean by that? Sure, there's no random access, but if you're storing pointers directly to the links themselves it shouldn't be necessary.

[edit] I remember what I said regarding lists. I remember saying that accessing an element by index is slow as you have to manually iterate through each link.

Zotoaster
19
Years of Service
User Offline
Joined: 20th Dec 2004
Location: Scotland
Posted: 4th Sep 2008 01:31
Quote: "But why use a solution to something that isn't necessary in the first place?"


I'm just putting the card on the table. Not saying he should use it where he is, but if he does want to use dynamic memory and is worried about not releasing it then that's a safe way to go.



Quote: "What do you mean by that? Sure, there's no random access, but if you're storing pointers directly to the links themselves it shouldn't be necessary."


True. I still prefer vectors though </crazy bias>

ionstream
20
Years of Service
User Offline
Joined: 4th Jul 2004
Location: Overweb
Posted: 4th Sep 2008 06:08
Thanks guys! How would I use std::vector<Sprite> ? If I did something like this:

Sprite sp;
sprites.push_back( sp );

Wouldn't "sp" be out of focus and deallocated as soon as the function ends?

If I knew more about trees maybe I could implement some kind of priority queue that is quick to find and delete sprites. Although right now if I used vectors I could just iterate through the loop when I want to delete something, I guess it can't be that slow considering there's only maybe 40 sprites in the list at the same time.

I think the worst part of my code is in EnemyController.cpp, what with all those enums and stuff. I think I'm going to replace sprite_templates[] with a static SpriteTemplate member variable in each enemy class, so that it creates it's own sprite (rather than leaving that switch statement to do it).

Benjamin
21
Years of Service
User Offline
Joined: 24th Nov 2002
Location: France
Posted: 4th Sep 2008 06:26
Quote: "Wouldn't "sp" be out of focus and deallocated as soon as the function ends?"

What happens is push_back() allocates a new Sprite object, and copies the data from sp into it. If you need the process more simple than just a memory copy then you'll need to write a copy constructor.

Also, I'd suggest that in class declarations you keep the same kind of identifiers grouped together (ie. types, variables, then functions) as it is usually easier to read.

Zotoaster
19
Years of Service
User Offline
Joined: 20th Dec 2004
Location: Scotland
Posted: 4th Sep 2008 06:53 Edited at: 4th Sep 2008 06:54
Quote: "Also, I'd suggest that in class declarations you keep the same kind of identifiers grouped together (ie. types, variables, then functions) as it is usually easier to read."


While we're on the subject, you should also put the most frequently used member variables at the top of all the variable declarations. The reason is that when compiled, the variables get compiled in the exact same order as they are made in C++. These tend to get into cache memory more often (I can't exactly remember how, but it happens none-the-less), and will result in better performance, since cache memory is lightning fast. Probably negligible at first, but hey, every little helps, and it's a good habit to get into.

Mista Wilson
16
Years of Service
User Offline
Joined: 27th Aug 2008
Location: Brisbane, Australia
Posted: 4th Sep 2008 08:50
There is nothing at all wrong with the structure of your code, it is encapsulated nicely into objects, and shows a very good understanding of OOP concepts, overall, an excellent job I must say.

the only problem that i can see is, as mentioned above, memory leaks.. that has to do with your memory assignment and how it's managed, because you are assigning memory on the heap, you would need to manage and track it so that it can properly be freed and cleaned up when needed... c++ native is unmanaged, so unlike a managed language(C# for instance) there is no "garbage collector" that will keep track of and free leaked heap memory for you.

One simple rule to remember if you are allocating your own dynamic memory in c++, is that every time you reserve a piece of memory for something, you should also have a corresponding piece of code somewhere to free it when its no longer needed.

Again, A great job on the structure of the code overall, definately not something that i would refer to as "craptastic" lol.

Good Luck with this and your future projects ionstream.

If it ain't broke.... DONT FIX IT !!!
ionstream
20
Years of Service
User Offline
Joined: 4th Jul 2004
Location: Overweb
Posted: 4th Sep 2008 09:47
Quote: "What happens is push_back() allocates a new Sprite object, and copies the data from sp into it. If you need the process more simple than just a memory copy then you'll need to write a copy constructor."


Aight cool, I guess I should write a copy constructor then.

I'm going to switch over to STL and the copy constructor technique then, so hopefully the memory leaks will disappear.

Quote: "Again, A great job on the structure of the code overall, definately not something that i would refer to as "craptastic" heh."


Thanks man! I will definitely try to clean up the design but keep everything separated like it for the most part is.

Thanks everyone for helping me out on this!

Login to post a reply

Server time is: 2024-09-30 05:40:20
Your offset time is: 2024-09-30 05:40:20