#172568 - zelbo - Tue Feb 16, 2010 2:07 am
I hope this is the right place for this post. I am currently working on a rogue-like for the DS. I am using this: http://roguebasin.roguelikedevelopment.org/index.php?title=How_to_Write_a_Roguelike_in_15_Steps as a rough guide, and only need to get my message system and 'look' function working before i move on to save/load. But before i do that, i would like someone who knows what they are doing to take a look at what i have so far and give me some advice or any tips.
Right now it's a big mess, and what i need is to make some of it more clean and elegant if possible. i have a file full of questions that are mostly notes to myself, and when i get to the point of not being able to answer one of them, i usually ask here. That file might give you an idea of where i am currently in my learning process.
Aside from the aforementioned message system and 'look' function, it pretty much does what i want it to at this point. So, even though i'm not against help with adding functionality, what i need now is to make what i already have smoother and more efficient. Things like using bitshifting and pointers where appropriate, and cutting down on my nested if loops (it's pretty much just if branches and for loops as far as the eye can see.)
I hope this post makes sense, i'm a bit congested right now.
If anyone is willing to take a look, i have a zip of the whole thing posted at google docs: https://docs.google.com/leaf?id=0B6agsPhpvqzJMzBlOWQ4MzItNTEzNy00YTRlLWFmMzQtYWE2ODQxZWJkMDFh&hl=en
#172569 - headspin - Tue Feb 16, 2010 2:33 am
You're better off breaking the issues you're having into smaller questions and code snippets rather than post your entire code and hope someone wades through it all. It's rare you will find a coder with the time to do that but you might be lucky.
_________________
Warhawk DS | Manic Miner: The Lost Levels | The Detective Game
#172571 - zelbo - Tue Feb 16, 2010 2:58 am
Ok, thanks. I figure it's worth a shot. I'm not having a specific problem right now (well, none that i'm ready to ask for help on just yet). Everything works, i just thought it would be a good time to look for bad habits before it gets too far. I think (hope) it's still small enough at this point that someone that is well versed in c++ can spot any glaring bad code. I guess it is a bit of a longshot, but hey, maybe someone wants to take a break from their own headaches and get distracted by something else for a minute. I don't really want anyone to read the whole thing, i was just hoping a few different people would glance at it and mention if they spot anything right away.
#172572 - Pete_Lockwood - Tue Feb 16, 2010 3:04 am
Stranger things have happened.
I downloaded it but I have an actual headache brought on by too much poker and Burnout Paradise :D
I may look at it some time. Possibly.
_________________
It's not an illusion, it just looks like one.
#172574 - Azenris - Tue Feb 16, 2010 3:14 am
Not your questions but, meh.
Doesn't having a virtual destructor mean a vtable is created for the class, even if you don't derive from it. All your classes have virtual destructors.
Personally in classes I make the variables private or public and use protected only when I really need.
Sometimes if's incrementing something can be slightly changed. like your line
| Code: |
| if(held) keytimer++; |
could be
_________________
My Homebrew Games
#172579 - Miked0801 - Tue Feb 16, 2010 8:33 pm
Ok, I've got some feedback after 5 minutes or so of looking. I can dive in more later if need be.
1. You have Magic Numbers everywhere. In Main, there are exactly 5 #defines, the rest are just numbers. Please, start using const int or static const ints for your numbers (not #defines). Right now, if you ever changed the size of your maps or the tilesets, you would have to modify by hand every single file - error prone and tedious.
2. You are coding in C with a few C++ conventions. If you are going to use C++, use its best features. Right now, everything is using naked arrays and naked pointers. Consider using Vectors or other appropriate containers or smart pointers for your data. Array bounds checking amonst other advantages cannot be overstated for ease of development.
3. Use const and references. Right now, you pass naked arrays - why not pass as const reference if it doesn't change and let the compiler do a little bit of work for you to prevents accidents.
4. Assert. I didn't see a single assert anywhere in your code. Add them now and save debug time later.
5. Consider using Boost Smart Pointers. Having data automatically clean up after itself is a wonderful thing. Never having to track through code looking for dangling pointers to doom is priceless.
6. IMHO, you are on track to making Main.cpp a large, monolithic class. Take a step back and be sure that main is doing one thing and doing it well. Make new helper classes to hide implementation whenever possible. If not new classes, at least functionize stuff better. Right now, there is a 5+ page function in main. That is too long for ease of reading and debugging. As an example, place your input accepter into its own class or at least consider if'ing off the masks and calling functions per action. That would cut most of the clutter I see there now.
7. Beware making a 3*64*64*sizeof(tile) in ITCM unless you really mean it. This would probably be better new'd to constant or dynamic sizes so that it goes into normal RAM.
8. Along the same lines, I see no obvious include for the "tile" class. It's in one of your includes, but I can't tell at a glance. Consider putting each of your shared classes into individual files for ease of reading and development. Local classes can remain outside of .h files.
9. Do NOT use 1 or 2 letter variable names. FORTRAN is dead. We have smart compilers now that allow us to write variables in English that make code readable. i or j for loops are ok (not great), but beyond that, you are just asking for trouble. iz, iy, ix? Can you tell me without context exactly what these are doing? How about q? You typed the name quadrant in a comment, why not use it as a variable name so that the comment is not needed?
10. Comments. Do comment when writing large blocks of code (you do this pretty well now). Comment on overall thoughts, not summerizing what the individual code lines are doing. If you feel that the code isn't readable without commenting on its functioning, you have a section of code that may need to be revisited for cleaner implementation. Sometimes, obtuse code is unavoidable for performance or space reasons and in these cases, dense comments whould abound. But that should be the exception and not the rule.
The stuff I've seen in your code is very common to programmers who work outside of team envirnoments and have not been coding for long. If you take my advice to heart, your code will show much, much better to future employers and will be much easier to read and maintain. Good luck on your game. I hope to see it completed.
#172582 - sajiimori - Tue Feb 16, 2010 10:21 pm
To elaborate and reinforce Mike's post:
The first smart pointer to master is boost::scoped_ptr, to mostly solve memory leaks. The second is std::auto_ptr, to mostly solve ambiguities with who is responsible for deleting objects. Third, write (or find) a template for smart pointers that are automatically nullified when the object they point to is deleted, to solve dangling pointers.
Don't feel guilty using std::vector. Worried about efficiency? Good! Write your own optimized containers over time, and gradually move away from standard containers. (Try writing one that's exactly as efficient as a normal array, but has safety checks in debug builds.) At any rate, don't use plain arrays as a crutch; I've seen that a lot, and it just holds you back.
#172584 - zelbo - Wed Feb 17, 2010 1:53 am
Awesome feedback! Thank you very much.
I am currently working on trimming those 3d arrays down. I've been trying to figure out how to pass an array as a pointer, and i think i can do it for a one dimensional, but the multi-dimensional ones are giving me trouble. If passing by reference is better, i'll need to figure that out. I am aware that i'm not using asserts, i've been meaning to figure that out, i suppose i'll need to move that to the top of my list at this point. The rest of it, i had no idea about any of that. I definitely have some research ahead of me, but at least now i have a better idea of what to look for.
Thank you, this is exactly the sort of feedback i was hoping for. I'll probably be back asking for help on some of these things, but i'm going to try to figure it out first.
#172596 - sajiimori - Thu Feb 18, 2010 7:51 pm
If you really want to pass arrays around, wrap them in structs. In C and C++, arrays are not first-class values and they have weird syntax and semantics.
Structs containing arrays are just plain values: you can assign to them whole (using the = operator), and pass or return them by value, pointer, or reference, all with syntax as straightforward as int.
#172602 - zelbo - Fri Feb 19, 2010 2:40 am
I'm trying to figure that out, the wrapping with structs and vectors, but haven't quite focused on it yet. But that's why i'm asking for this kind of information before i add anymore functionality. I am learning as i go, and at this point i need to know what to learn.
So right now i'm trying to get all my input handling moved out of main and into functions in my game object so i can starting fleshing it out as a finite state machine. To do that, i need to get a better handle on passing pointers and references, both how to do each and when to favor one over the other. It's a little confusing, but i think once i get a better handle on pointers, the rest of this should start falling into place.
But now i'm noticing that i screwed up the system tying sprites to objects because i didn't understand it, and i need to also get that fixed to finish moving stuff out of main.
I think i have also made a bit of progress on cleaning up my magic numbers, but still have a long way to go there. Once i get main cleared out, i'm going to tackle arrays, while still poking at the magic numbers problem. i think i can start working with 1d arrays now, but i'm not quite ready to implement them until i get some other stuff cleaned up first.
Having a little trouble knowing when i should be using asserts, so i think i'm just going to try using them everywhere i can think of. am i right in thinking asserts are better over-used than under-used?
I have a lot of questions, but i'm still trying to figure some of them out before asking for help. I think one thing i am having trouble with is my movement checking system, here are some of the ideas i've been tossing around:
1) using arrays of 'tileinfo' structs for terrain/items
2) using bitsets for the terrain and item tiles
3) just using the map to figure out each tile on the fly (i'm guessing this is not a good idea, but i can't really say why at the mo')
i think i've settled on an array of monster pointers for the creatures, but am open to suggestions there. Considering that an array for that sort of thing would still take up space even with most of the elements in it being empty (i think), i've been thinking about a linked list, but that's another thing i haven't wrapped my head around yet.
I guess i'm not in a hurry to nail this all down yet, since i'm still cleaning up a lot of other stuff, but i think i'll want to settle on something before i move on to file i/o.
Now i think i've gotten off track with this post, so i'll go do some more research. Again, thank you all very much for getting me pointed in the right direction. Your feedback has been extremely helpful.
#172603 - headspin - Fri Feb 19, 2010 3:15 am
Well I stand corrected! This has got to be the best free code review website in the world! Bookmarked!
_________________
Warhawk DS | Manic Miner: The Lost Levels | The Detective Game
#172606 - sajiimori - Fri Feb 19, 2010 6:27 am
Quick rule of thumb regarding when to pass references vs pointers:
1) Never pass non-const references.
2) Pass by const reference only as an optimization to avoid copying large objects (larger than an int).
3) Pass a pointer to allow the function to modify the original object or hold onto its address.
| Code: |
void examples()
{
Object obj;
// "I'm either passing you a copy, or a const reference;
// I don't care which, because you won't modify my copy,
// and you won't hold onto its address."
functionA(obj);
// "& means I'm giving you the address of my object. You
// need the address for some reason: you're either going
// to modify my object or hold onto its address over time."
functionB(&obj);
}
|
The point is that seeing & gives you a hint that the address matters. In growing programs, the most confusing thing is who's modifying what: it's important to see clearly where variables might be getting modified.
#172616 - Miked0801 - Fri Feb 19, 2010 8:26 pm
Some further answers to your questions:
On asserts - they usually live at the top of a function and double check incoming data to make sure that it is valid. An easy example is pointers. One almost always asserts pointers against null to prevent null->garbage type accidents. Alos, if a value can only be in a certain range - say you pass in screen coordinates, you assert their range. So in that case assert(screenX >=0 && screenX <= 239)...
Basically, use asserts to lock down where bugs are coming from. If you find yourself wanting to put asserts in the middle of a function, that probably means you are making your functions too long and having them do too much.
For your mosters, look into STL containers. A straight array of pointers is not the right solution. If it's a list of monsters that grow and shrink over time, use a list. If you change your mind later, most STL containers have very similiar interfaces so it is easy enough later on to change to a Vector or something else.
#172621 - zelbo - Sat Feb 20, 2010 7:34 am
| Quote: |
| If it's a list of monsters that grow and shrink over time, use a list. |
Yeah, i was thinking that's what i would have to do. My problem there is then, isn't a list a key/entry type of deal? When i'm trying to figure out what is near my player object (or other objects, for that matter), with an array i can use x and y. With a list, would i just need to run through the whole thing and check the position values of each entry? Wouldn't that cause a bottleneck when each creature is doing that every cycle?
Again, thank you guys so much for all your help. I'm totally giddy about the prospect of not adding any new functionality to my game. I'm some sort of freak, aren't i?
#172624 - vuurrobin - Sat Feb 20, 2010 1:08 pm
a (linked) list contains just values like arrays and vectors. a map contains key/value pairs.
_________________
my blog:
http://vuurrobin.100webcustomers.com/
#172626 - sajiimori - Sat Feb 20, 2010 7:38 pm
Roguelikes are weird because there's a limited set of spaces where enemies can be standing. Most games let characters move more freely, so a 2D array of enemies doesn't usually make sense.
But yes, in a roguelike, it would make sense to use a 2D array to represent the world, where each entry is a struct describing the contents of the tile (including a pointer to any enemy that might be standing there).
Since only a few tiles will contain enemies, the tile structs shouldn't contain whole enemy structs -- just pointers to enemies, so you only waste a pointer for tiles that don't currently contain enemies.
The easiest way (I can think of right now) to manage the actual enemy structs (since they have to exist somewhere in memory) would be to have the tiles own their contained enemy, using std::auto_ptr.
| Code: |
struct Tile
{
// Should it be "Character" instead of "Enemy"?
std::auto_ptr<Enemy> enemy;
// (other properties/contents of a tile...)
};
void createEnemyInTile(Tile* t)
{
// Careful: std::auto_ptr's reset() method deletes its
// existing contents, so if an enemy was standing there,
// this function kills it instantly.
t->enemy.reset(new Enemy);
}
void moveEnemy(Tile* from, Tile* to)
{
// std::auto_ptr's special assignment operator clears the
// source pointer and deletes any existing enemy in the
// destination.
to->enemy = from->enemy;
}
void destroyEnemyInTile(Tile* t)
{
// reset(NULL) also has the same effect.
t->enemy.reset();
} |
std::auto_ptr makes it unlikely that you'll ever accidentally forget to delete an enemy, or that you'll delete it twice, or that you'll use a pointer to a deleted enemy. Easy! =)
Advanced note regarding forward declarations: std::auto_ptr is dangerous in combination with forward-declared types. When it frees the object, it won't necessarily run its destructor. Write your own replacement that guarantees correct behavior, and never use std::auto_ptr again.
#172627 - sajiimori - Sat Feb 20, 2010 7:56 pm
Oh, and I guess:
| Code: |
class World
{
public:
Tile* tile(int x, int y)
{
assert(x >= 0 && x < WORLD_WIDTH);
assert(y >= 0 && y < WORLD_HEIGHT);
return &tiles[y][x];
}
private:
Tile tiles[WORLD_HEIGHT][WORLD_WIDTH];
}; |
That's much better than an exposed 2D array, but calling code is still responsible for checking x and y before getting a tile. If they make a mistake, it crashes the game with a failed assert. At some point, I'd consider whether this would make your life easier:
| Code: |
const Tile kSolidTile =
{
// (properties of a tile that you can never walk through,
// and has no interesting qualities.)
};
Tile* World::tile(int x, int y)
{
if(x < 0 || x >= WORLD_WIDTH ||
y < 0 || y >= WORLD_HEIGHT)
{
return &kSolidTile;
}
return &tiles[y][x];
} |
#172682 - zelbo - Thu Feb 25, 2010 2:32 am
Ok, i've been seriously attacking the magic numbers problem that MikeD mentioned. Now i'm having a problem.
| Quote: |
| Please, start using const int or static const ints for your numbers (not #defines). |
I've looked around at various FAQs and at the reference material i have on hand, but i can't figure this one out: How do i centralize my constants? Right now, i have a copy of each constant in each file that uses them. And i'm pretty sure that's not the right way to go about it. I would put them into headers and include the appropriate ones, but data in headers is a no-no. I've tried putting them in various places, but i just can't figure this one out. And my list of constants is growing. I would make them all defines, as i was starting to do, but more than once i've heard that #defines should be avoided.
I swear i'm trying to figure this stuff out on my own, but sometimes i just need to ask. Everyone has been very helpful. If i can get this game made, i should have a lot less questions when i start working on my MOO2 clone.
#172683 - wintermute - Thu Feb 25, 2010 4:36 am
Constants aren't data, headers are precisely where constants should be.
#defines don't need to be avoided, they're very powerful when you know what you're doing. The basic problem with them is that when you make a mistake then the error the compiler gives you (if it does) refers to the line and source file where the macro was used and not in the header where the macro is defined.
The advantage of using static const <type> = <value> in your header for constant values is type safety. You're telling the compiler precisely what kind of value you're using and giving it a chance to warn or error if you attempt to assign it to a variable of a different type.
_________________
devkitPro - professional toolchains at amateur prices
devkitPro IRC support
Personal Blog
Last edited by wintermute on Thu Feb 25, 2010 10:24 pm; edited 1 time in total
#172685 - elwing - Thu Feb 25, 2010 7:32 am
| wintermute wrote: |
| The advantage of using static const <type> = <value> in your header for constant values is type safety. |
you sometime specifically want some constant to not be type safe :)
#172696 - Miked0801 - Thu Feb 25, 2010 6:48 pm
String concatination of typenames are one of the few places where we still use defines to make table generation a bit cleaner to read and maintain. ## is your friend. Occasional use as typeless inlines as well, but this is dangerous and where templates can do better. Also, ... can be useful when passing in multiple, typeless params for vargs stuff.
But for just constants, use const and for most inlines, use inline.
#172703 - zelbo - Thu Feb 25, 2010 11:28 pm
cool beans. now i just need to get on the ball with smart pointers, containers for my arrays, asserts, nailing down a format for my tiles, organizing my code better, implementing message system and look commands, and i'll be ready to move on to file I/O. Yay me!
Everyone has been massively helpful with all my n00b questions. Thank you all very much.
#172705 - sajiimori - Thu Feb 25, 2010 11:48 pm
| elwing wrote: |
| wintermute wrote: | | The advantage of using static const <type> = <value> in your header for constant values is type safety. |
you sometime specifically want some constant to not be type safe :) |
And as a corollary, #defines can be 100% typesafe, such as: | Code: |
| #define MY_VALUE (Vec3(12, 34, 56)) |
There's nothing you can do to make that be interpreted as anything other than a Vec3.
I'm not particularly anti-#define. Defining non-integer constant variables in headers actually does result in duplicate data, plus it causes CPU overhead to load the value from memory. C++ has a special rule for integer constants, which is confusing.
I only have 2 suggestions for picking "one true way" of defining constants (where you don't have to keep second-guessing yourself every time):
1. always use #defines, or
2. always use static inline functions that return a constant value.
The function approach is not traditional, but oddly, it's the best way I know of to guarantee minimal CPU overhead, minimal duplication of data, and proper namespace scoping, without having to remember any special rules.
Oh... unless you want to use your constant as an array size or a template parameter. Then you have to use a #define, an enum, or a static const int. *sigh*
#defines are the only "one true way" that meets all the requirements, except proper namespacing scoping.
Pick your poison!