Class Creation and Vector ( Array ) Question

Started by
10 comments, last by Zahlman 18 years, 4 months ago
Ok, like some of you know im busy learning a bit of C++ and im in the middle of creating a small text adventure game. I have created a class [Location] which will allow me to create instances of locations for my game. I am also trying to use vectors to store this information. Here is my code.

#include <cstdlib>
#include <iostream>
#include <vector>

using namespace std;

class Location // New Class
{
protected:
          vector<char> m_VerExits; 
          vector<char>::iterator m_VerExits;    

public:
       Location(char *m_RoomTitle, char *m_RoomDescription); // Constructor with variables
       ~Location(); // Destructor.
       
       char m_RoomTitle;
       char m_RoomDescription;
       char exit;
       
       char toString(char m_RoomTitle);
       void addExit(vector<char> m_VerExits, char exit);
       void removeExit(vector<char> m_VerExits, char exit);
       void getExits();
       char getTitle(char m_RoomTitle);
       char setTitle(char roomTitle);
       char getDescription(char m_RoomDescription);
       char setDescription(char roomDescription);
};

       Location::Location(char *m_RoomTitle, char *m_RoomDescription)
       {
              // m_RoomTitle = "Room1";
               //m_RoomDescription = "Room1 Description";
       }
     
       char Location::toString(char m_RoomTitle)
       {
             return m_RoomTitle;
       }
  /////////////////////////////////////////////////////////////////////////   
       void addExit(vector<char> m_VerExits, char exit)
       {
            m_VerExits.push_front(exit);
       }
     
       void removeExit(vector<char> m_VerExits, char exit)
       {
            if (m_VerExits.contains (exit))
            {
                    m_VerExits.Delete(exit);
            }
       }
   ///////////////////////////////////////////////////////////////////////
       char Location::getTitle(char m_RoomTitle)
       {
            return m_RoomTitle;
       }
     
       char Location::setTitle(char roomTitle )
       {
            m_RoomTitle = roomTitle;
       }
     
       char Location::getDescription(char m_RoomDescription)
       {
            return m_RoomDescription;
       }
       
       char Location::setDescription(char roomDescription)
       {
            m_RoomDescription = roomDescription;
       }
                 
     
int main(int argc, char *argv[])
{
}

1) Please have a look at my class definition. Does it make sense?? 2) I am battling to create/destory another 'element' for my vector m_VecExits.. How do I do this? Thanks! [Edited by - Megafox on December 1, 2005 6:58:20 AM]
Advertisement
Quote:
2) I am battling to create/destory another 'element' for my vector m_VecExits.. How do I do this?


The vector class has four functions; push_back(<item>), pop_back() and push_front(<item>), pop_front() that you might find useful

Its a lot of errors in your code by the way, your vector, and the vector::iterator has the same name among other things

[Edited by - pulpfist on December 1, 2005 7:49:37 AM]
Also, you should change your parameters in the following functions to use references:

       void addExit(vector<char>& m_VerExits, char exit)       {            m_VerExits.push_front(exit);       }            void removeExit(vector<char>& m_VerExits, char exit)       {            if (m_VerExits.contains (exit))            {                    m_VerExits.Delete(exit);            }       }

Otherwise they're not performing push_front etc on the vector you supply to the function, just the copy. There could (probably) be more, but I'm justp passing through.

EDIT: Hunh, formatting is a little different under the new scheme.
[ search: google ][ programming: msdn | boost | opengl ][ languages: nihongo ]
@pulpfist... yeah i just realised that myself.. i have changed my iterator name...i wasnt aware that it should not have the same name, still learning ;)

vector<char>::iterator m_VerExits_Iterator;

also i presume i need to write a function to loop through the vectors in the removeexit function.. but that is for tmr...
Quote:Original post by Megafox
1) Please have a look at my class definition. Does it make sense??
2) I am battling to create/destory another 'element' for my vector m_VecExits.. How do I do this?

Thanks!


I added comments to your code with my opinion. These comments are intended as constructive criticism that I hope you find useful.

#include <cstdlib>#include <iostream>#include <vector>/**** I personally prefer not to use "using namespace"      if you really don't want to type "std::vector" everywhere      try instead:using std::vector; ****/using namespace std;class Location // New Class{protected:/**** I'm confused here for two reasons:      - I understand m_VerExits is supposed to represent exits of some sort        but what does 'Ver' mean? Try to come up with a more descriptive name      - Why a vector of chars? Do you not mean to use a vector of std::string         instead? Or is an exit somehow represented by a single character?  ****/          vector<char> m_VerExits;/**** You don't use this iterator anywhere. Usually, iterators are created        as local variables in the functions that need them. I would remove it,       if only because it has the same name as the previous variable and       therefore won't compile. ****/          vector<char>::iterator m_VerExits;    public:/**** People that use the "m_" prefix use it normally to indicate members of      a class. Here they are parameters of the constructor, so the "m_" prefix      is inappropriate and very dangerous (it hides the member variables!). ****/       Location(char *m_RoomTitle, char *m_RoomDescription); // Constructor with variables       ~Location(); // Destructor.       /**** You seem to be confusing "char *" and "char". Only "char" means      a single character, which almost certainly is not what you want.      Since you use C++, use std::string to represent text.      Also try making all your member variables private first. Only in      exceptional situations, it's worth to make member variables public ****/       char m_RoomTitle;       char m_RoomDescription;/**** Is exit supposed to be text?  ****/       char exit;       /**** toString is normally used to convert the current state of      an object to a string, and has no parameters. If you mean       to have the toString function return the room title, it need      not be a parameter, because it can access this value as it is      a member function. And again you probably need std::string. ****/       char toString(char m_RoomTitle);/**** Again only a char (std::string?) exit parameter is required.      the m_VerExits member can be accessed directly from the function. ****/        void addExit(vector<char> m_VerExits, char exit);/**** And again. I'm sure you get the idea. ****/       void removeExit(vector<char> m_VerExits, char exit);/**** Very confusing name, it indicates something can be obtained (get)      but it returns nothing. What is this supposed to do ****/       void getExits();/**** And again. I'm sure you get the idea. ****/       char getTitle(char m_RoomTitle);/**** Why does this "set" function return a value?. ****/       char setTitle(char roomTitle);       char getDescription(char m_RoomDescription);       char setDescription(char roomDescription);};       Location::Location(char *m_RoomTitle, char *m_RoomDescription)       {/**** These lines are commented because they don't compile due to       char and char* not being of the same type. As stated before:       std::string solves all your problems (i.e. do make sure you remove the       trailing m_ from the constructor parameters, or remove the parameters       altogether if you're just assigning constants like this)  ****/              // m_RoomTitle = "Room1";               //m_RoomDescription = "Room1 Description";       }            char Location::toString(char m_RoomTitle)       {             return m_RoomTitle;       }  /////////////////////////////////////////////////////////////////////////          void addExit(vector<char> m_VerExits, char exit)       {/**** I'm not sure if "push_front" exists for std::vector. If you really need      to add to front and back use std::deque instead (or just use push_back) ****/            m_VerExits.push_front(exit);       }            void removeExit(vector<char> m_VerExits, char exit)       {/**** If you need random deletetion alot (and guaranteed ordering) std::list       is better than std::vector and std::deque  ****/            if (m_VerExits.contains (exit))            {                    m_VerExits.Delete(exit);            }       }   ///////////////////////////////////////////////////////////////////////       char Location::getTitle(char m_RoomTitle)       {            return m_RoomTitle;       }            char Location::setTitle(char roomTitle )       {            m_RoomTitle = roomTitle;       }            char Location::getDescription(char m_RoomDescription)       {            return m_RoomDescription;       }              char Location::setDescription(char roomDescription)       {            m_RoomDescription = roomDescription;       }                      int main(int argc, char *argv[]){/**** This obviously needs contents :) ****/}


Main comments (summary if you wish):
- You seem to need std::string wherever you use char or char*.
- You seem to have the misconception that a function needs member variables as parameter to be able to access them. Unfortunately, this will have the opposite effect. Making a parameter m_roomTitle will hide the object member m_roomTitle and the value assignment/reading is done on the wrong variable. This is solved simply by removing all function parameters you have that start with m_.

Tom
Definitely better than some things I've seen around here :)

But yes, if you are smart enough to use std::vector, you should be smart enough to use std::string.

I'm not sure what kind of text entries you want to store in your "m_verExits" either; will they be the names of other rooms you want to go to? If so, there will still be a bit of work involved in finding the other Location. Or perhaps you wanted to associate a separate name (like 'East') with each exit, so that you can look it up based on what the user types in (e.g. 'go east'). In either of these cases, you may find std::map useful.

Putting that aside, though, here's some cleanup:

#include <cstdlib>#include <iostream>#include <vector>#include <string>using namespace std;class Location {  // Chances are good that the Location class won't need to be inherited from.  // So for now I'm leaving this part private:  vector<string> m_exits;   // I don't see what the 'exit' member was supposed to be for.  // I'm also going to put the title and description up here, because there is  // no need for direct access to them from outside - they should only be  // touched when you create a Location, and when you print it out.  string m_title;  string m_description; // "room" doesn't have any descriptive value in  // these names, so I took it out.  public:  Location(const string& title, const string& description) :   m_title(title), m_description(description) {}  // I defined the constructor within the class definition because there  // is no actual necessary body; the work is done entirely by the initializer  // list. This is more efficient (not important) and also cleaner (very nice).  // Note the input strings are passed by const reference; this is the usual  // way of passing in objects.  // No destructor is needed because the default will call the destructor for  // each member, and vectors and strings both already know how to clean  // themselves up.  // In C++ we do not use toString normally; it can be made to work, but  // it is not the normal idiom. Instead you can do this sort of thing:  ostream& printTo(ostream& os) const;  // and there will be some fleshing out of it below...  void addExit(const string& exit);  void removeExit(const string& exit);  // We definitely don't want get/set for title and description. Why would  // these change over time? As for getting them, we only want to get them  // for display purposes.};// Here's what printTo looks like...ostream& Location::printTo(ostream& os) const {  os << m_title << endl;  os << m_description << endl;  os << "Exits:";  // Iterate over exits and print them out.  for (vector<string>::iterator it = m_exits.begin(); it != m_exits.end(); ++it) {    os << ' ' << *it;  }  return os;}// And now here's something to finish that off:ostream& operator<<(ostream& os, const Location& loc) {  return loc.printTo(os);}// Now you can do things like "cout << myLocation << endl;" ;)
Thank you so much for your replies. Thats why I love being here, always people willing to help a newb like me... ;) I will be using both to get a solution going for my location class.

What im trying to do is create a class whereby i can basically enter a room no / description and define an exit. This info will then be hardcoded by me in main() for now, text file loading later.

I presume this is the most efficient way to go since I would be able to create a dungeon in my main() by saying

location ("Room1", "This is a cold room", EAST") // this is just conceptual of course ;)
etc etc

btw im an oldhand VB programmer and im finding it quite a steep learning curve.
I have decided to step back and have a look at what im trying to accoplish here. I was trying to use classes and vectors right off the bat without actually understanding how they work. So what i did was I created a plain and simple class, derived 2 objects from the class and coutted the results to the screen. I think this is an important step to what im trying to achieve!

Here is the code... [btw how do I encapsulate my code into code boxes on the screen??? ]

#include <cstdlib>#include <iostream>using namespace std;class Location {      public:             char* Room;             char* RoomDescription;             char* Exit;             Location (char* Room, char* RoomDescription, char* Exit);};Location::Location (char* ret_Room, char* ret_RoomDescription, char* ret_Exit){                  Room = ret_Room;                  RoomDescription = ret_RoomDescription;                  Exit = ret_Exit;                  }class Character{      private:              int Health;              int Mana;              int Strength;            public:             char* Name;             char* Surname;             char* Race;             char* Class;             char* Profession;             Character (char* Name, char* Surname, char* Race, char* Class, char* Profession, int Health, int Mana, int Strength );};             Character::Character (char* ret_Name, char* ret_Surname, char* ret_Race, char* ret_Class, char* ret_Profession, int ret_Health, int ret_Mana, int ret_Strength){             Name = ret_Name;             Surname = ret_Surname;             Race = ret_Race;             Class = ret_Class;             Profession = ret_Profession;             Health = ret_Health;             Mana = ret_Mana;             Strength = ret_Strength;}int main(){         char* NameInsert;      char* RaceInsert;      system("PAUSE");      Location loc1 ("Room1","it is a cold room","NORTH");      Location loc2 ("Room2","it is a hot room","SOUTH");            cout << "ROOM(1) : This is " << loc1.Room << " and " << loc1.RoomDescription << " and the exit is to the " << loc1.Exit <<endl;      cout << "ROOM(2) : This is " << loc2.Room << " and " << loc2.RoomDescription << " and the exit is to the " << loc2.Exit <<endl;            system("PAUSE");            cout << " Welcome adventurer!! What is your name ??";      cin >> NameInsert;            Character.Name = NameInsert;      cout<< Character.Name << " Now I need you too choose a Race."            system("PAUSE");}


My Problem is that i cant seen to populate the Character.Name field from NameInsert. The compiler gives me an error. Is there something I have missed?

[Edited by - Megafox on December 5, 2005 6:51:47 AM]
Quote:Original post by Zahlman
But yes, if you are smart enough to use std::vector, you should be smart enough to use std::string.

Please, please, please, replace your evil char *s with std::strings. Do you want to be dealing with chunks of text which encapsulate size, copying, appending etc. and just work™ or raw arrays of characters which will require you to constantly allocate and deallocate chunks of memory and manually copy the contents?

Your compiler error is caused by the fact that Character is the name of a class, not an instance. Just like Location is a class and loc1 and loc2 are instances.

[source] tags can be used to post code in pretty boxes. This is documented in the FAQ.

Enigma
You have to strcpy() strings, you can't assign pointers like that. You see, you are probably assingning pointers to items on the stack, and when the stack frame is lost, those pointers are left dangling to some location on the stack that is no longer valid, and it crashes.



char buffer[128];

strcpy(buffer,s);




You would probably be better to use std::string, but sometimes char buffers are a better solution because STL grinds the heap heavily.

Note that string buffers like this are a big source of serious security problems so it's best to avoid them until you know how to deal with.

This topic is closed to new replies.

Advertisement