Jump to content

  • Log In with Google      Sign In   
  • Create Account

Awesome job so far everyone! Please give us your feedback on how our article efforts are going. We still need more finished articles for our May contest theme: Remake the Classics

#Actualmrjones

Posted 12 August 2012 - 03:56 PM

I didn't locate the crash and didn't run the code because I didn't have SDL development libraries installed (only SDL2), but I have some general recommendations that might help. These are really quite general and you have probably of them, but I'll reiterate them anyway.

First of all, the class inheritance is a bit of a mess and memory seems to be leaking. The worst is that almost everything seems to derive from LoadFiles and its constructor loads all images, fonts etc. Each time anything derived from LoadFiles is created, and these classes seem to be many in number, all images and fonts probably get loaded again and again. I'm not 100% sure given that I've only taken a glance at the code, but it really seems to be so.

Secondly, there seems to be lot of inheritance. It is better to use composition in most cases. Try looking up composition vs inheritance. There was a good post about it on GameDev, but I can't find it at the moment.

There also seems to be a bit of duplicate code. For example Item::setDead(). Item could just as easily have boolean dead member and set it instead of it being marked abstract. All the child classes are just doing the same. Just try to find code that duplicates itself and remove it. It's a slow process at first and it's often easier to just copy-paste, but it will hurt a lot later even when trying to make sense of your own code that is months or years old. Also, the less code you have to read the more obvious the errors will become. I don't mean shortening the names of variables, but the logical duplicates where a new function could be used in more than one place or even when it makes logically more sense to keep it separately.

Now, the pointer-hell. There are a lot of pointers, so many I lost track. Whenever possible, try to use references, full memory copies and stack allocated variables. In my experience pointers are almost always bad unless you need to optimize something. And even then the pointers should be local.

There are also many references to pointers. A reference is technically a pointer anyway, with the restriction that it can not be reassigned to point to another location. For example instead of Cursor* &cursor it makes sense to use either Cursor* cursor or preferably Cursor& cursor since the pointer value is not reassigned and it would be a bad idea to do so anyway.

Then there is the problem with rendering code being mixed with the game state. Usually it is best to have the underlying structure separately and provide different views for it, but it's already way out of scope of what I planned to say at first and there's plenty said about it elsewhere.

I hope you get something out of my recommendations. Overall it's not bad for a first project at all. I wish I could be more thorough, but architecture and pointers are a huge subject and unfortunately without running the code seeking crash is like searching random uninitialized pointer from heap of bees.

Edit: Sorry as I am probably repeating much said above as I started writing when no other posts were made. I'm slow.

#1mrjones

Posted 12 August 2012 - 03:54 PM

I didn't locate the crash and didn't run the code because I didn't have SDL development libraries installed (only SDL2), but I have some general recommendations that might help. These are really quite general and you have probably of them, but I'll reiterate them anyway.

First of all, the class inheritance is a bit of a mess and memory seems to be leaking. The worst is that almost everything seems to derive from LoadFiles and its constructor loads all images, fonts etc. Each time anything derived from LoadFiles is created, and these classes seem to be many in number, all images and fonts probably get loaded again and again. I'm not 100% sure given that I've only taken a glance at the code, but it really seems to be so.

Secondly, there seems to be lot of inheritance. It is better to use composition in most cases. Try looking up composition vs inheritance. There was a good post about it on GameDev, but I can't find it at the moment.

There also seems to be a bit of duplicate code. For example Item::setDead(). Item could just as easily have boolean dead member and set it instead of it being marked abstract. All the child classes are just doing the same. Just try to find code that duplicates itself and remove it. It's a slow process at first and it's often easier to just copy-paste, but it will hurt a lot later even when trying to make sense of your own code that is months or years old. Also, the less code you have to read the more obvious the errors will become. I don't mean shortening the names of variables, but the logical duplicates where a new function could be used in more than one place or even when it makes logically more sense to keep it separately.

Now, the pointer-hell. There are a lot of pointers, so many I lost track. Whenever possible, try to use references, full memory copies and stack allocated variables. In my experience pointers are almost always bad unless you need to optimize something. And even then the pointers should be local.

There are also many references to pointers. A reference is technically a pointer anyway, with the restriction that it can not be reassigned to point to another location. For example instead of Cursor* &cursor it makes sense to use either Cursor* cursor or preferably Cursor& cursor since the pointer value is not reassigned and it would be a bad idea to do so anyway.

Then there is the problem with rendering code being mixed with the game state. Usually it is best to have the underlying structure separately and provide different views for it, but it's already way out of scope of what I planned to say at first and there's plenty said about it elsewhere.

I hope you get something out of my recommendations. Overall it's not bad for a first project at all. I wish I could be more thorough, but architecture and pointers are a huge subject and unfortunately without running the code seeking crash is like searching random uninitialized pointer from heap of bees.

PARTNERS