Sign in to follow this  
XDigital

Really Annoying

Recommended Posts

Hi contestants, do you know, what's really annoying? The further I get the more I realize, that I have really no clue about C++. There is a lot of math, in the game right know - like transformations, vector algebra, lots of sin and cos, I'd like to add some serious physics - but I have no clue, why I can't do things like: typedef struct{ char *tribe[3]; } CHARACTER; CHARACTER character; character.tribe[0] = "Saxonia"; It took to me the whole evening without any results. And I really feel ashamed about it, because stuff like this really belongs to the basics... And even if you have an answer to this little problem (I know everybody of you have), I fear that I won't understand it. Anyway. Have to make a workaround. Wish you happy coding. Best regards XDigital

Share this post


Link to post
Share on other sites
The problem in your example is that there is no memory allocated to receive the string Saxonia. The char* tribe[0] is initialized to... -- it is not initialized at all!

So something like this would work better:

character.tribe[0] = malloc( 256 );
strcpy( character.tribe[0], "Saxonia" );


The cause of the problem is (probably) that this is more of C-style programming than of C++ or Object-Oriented programming. To move one step in the right direction, you could provide a constructor for CHARACTER that allocated the memory:

struct Character
{
char* name;

Character( const char* _name )
{
name = malloc( strlen( _name ) );
strcpy( name, _name );
}
};

This would disallow the wrong behaviour by compiler semantics, i.e. this:

Character character;

would not be allowed and generate a compiler error, while this

Character character( "Saxonia" );

would produce the correct result. This way invalid assignments like in your example simply cannot be constructed.

If you'd consider this, than please use the class-keyword instead of struct.

Good luck; greetz, Illco.

Share this post


Link to post
Share on other sites
Thanks,
the struct doesn't work, neither with malloc nor with the constructor. I tried the class stuff. This seems to be quite smarter than the struct stuff and works very fine:

class Character{
private:
public:
char *name;
char *gender;
char *race;
char *tribe;
char *profession;
char *level;
int num_level;

void SetName(char *n);
void SetGender(int num);
void SetRace(int num);
void SetTribe(int num);
void SetProfession(int num);
Character(void);
~Character(void);
};



and for example

//---------------------------------------------------------------------------
void Character::SetRace(int num){
//---------------------------------------------------------------------------
char *_race[] = {"Troll","Human","Albien"};
race = (char*)malloc(strlen(_race[num]));
strcpy(race,_race[num]);
}




Thanks for the "smack head" - XDigital

Share this post


Link to post
Share on other sites
Before going any further with your game I would recommend you learn how to program in C or C++ and perhaps read up on good coding practices before going any further. this would probably solve some of this very problems you are experiencing.

As for your example of "things not working" using malloc in C++ is generally bad (although in some cases it required). A better solution for storing the character information would be to create an enum of the types, then store that enum value in the character class. When the name is needed call a helper function to receive it. This will save memory (and potentially prevent leaks), help with performance, and is cleaner.

i.e.


typedef enum
{
CharacterType_Saxonia = 0,
CharacterType_Angle
} CharacterType;

class Character
{
CharacterType m_myCharacterType;
};

void Character::SetRace(CharacterType ct) { m_myCharacterType=ct; }
CharacterType Character::GetRace(void) { return m_myCharacterType; };


Now you'd be able to compare types of characters without string comparing...If you wanted to get the name of a specific type, you could do something like this:

class Character
{
static char* GetRaceName(CharacterType ct)
{
if (ct == CharacterType_Saxonia)
{
return "Saxonian";
}

return "Unknown"; // Alternatively you could return NULL (if you're going to handle it properly.
}
};

Wherever you need to access the race you simply type Character::GetRaceName (or could be split into a separate function) and it returns the name. Using enums/macros/constants is much easier than string copying text all over the place, saves memory, and is generally considered more efficient. Please let me know if this helps.

Share this post


Link to post
Share on other sites
I would reccommend reading up on the std::string class. It will *just work* when you assign to it like that - without having to fiddle with memory.

Good luck on your entry :)

Share this post


Link to post
Share on other sites
XDigital, everyone above me raises valid points and definitely take their posts to heart... However, the code you presented compiles fine for me on gcc... The only thing that's somewhat in error is that instead of "char* tribes[3]", you would want "const char* tribes[3]", since when you do the assignment character.tribes[0] = "Saxonia", the string literal, "Saxonia" should have type (const char*). When you use the term "Saxonia" in your code, the compiler will place the string "Saxonia" somewhere in your final executable, and when you do character.tribes[0] = "Saxonia", you are simply setting tribes[0] to the address of the constant "Saxonia", and there is nothing wrong with this.

But you may need "const char*" because you should't change that string, it's assumed to be constant by the compiler (if you change it, another part of your code may have name = "Saxonia" and in this case the memory to store "Saxonia" may be the same memory used to store "Saxonia" before, so if you change it in one place you'll (probably unintentionally) change it in all places. Bottom line, string literals are const char*, don't change them, and match your types up for assignments.

It probably would have helped if you had provided the error or warning that code was giving you when you tried to compile it.

Share this post


Link to post
Share on other sites
Thank you all for your hints and tricks. It seems that there are tons of results for this simple assignment.
@Paul: Well, as I said before - I tried the whole evening to get this thing running. And one of the biggest problem was, that there occured no error. The char array appeared to be NULL after assignment. But nevertheless the Init function for character generation runs now without struct but with class. But some of you said I should not use malloc. Now, what is the difference between malloc and new? I always thought memory allocation is memory allocation *tearhear*. OK, lets stop with beginner sessions and get back to work. The dragon is still standing and loads of particles have to be added.

There will be a rating for techniques, won't it? I was thinking about sending the code together with the game afterwards... Maybe I shouldn't.
Rgds
XDigital

p.s. an the winner in category memory leak is.........

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
I would reccommend reading up on the std::string class. It will *just work* when you assign to it like that - without having to fiddle with memory.

Good luck on your entry :)


Emphasis is mine. const char* or char* and explicit allocation/deallocation are dangerous to use, while std::string and it's RAII mechanism is perfectly safe. You don't leak memory with a std::string.

Same, arrays should be represented using std::vector - for the same exact reason.

In your example, it looks like this:
struct CHARACTER
{
std::vector<std::string> tribes;
}

CHARACTER character;
character.tribes.push_back("Saxonia");

This would be more elegant with some encapsulation:
class Character
{
std::vector<std::string> tribes;
public:
Character() { } // nothing in tribes yet
~Character() { } // the vector is automagically destroyed

// add a trib to the tribe list
void addTribe(const std::string& tribe)
{
tribes.push_back(tribe);
}
};
Character character;
character.addTribe("Saxonia");

Ultimately, you code is easier to read, safer to use, and less prone to memory leaks.

Regards,

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
I would reccommend reading up on the std::string class. It will *just work* when you assign to it like that - without having to fiddle with memory.

Good luck on your entry :)


Agreed. To paraphrase a signature I saw on here once: if you are a beginner and have char* in your code, you have a bug.

Share this post


Link to post
Share on other sites
Sign in to follow this