Jump to content
  • Advertisement
Sign in to follow this  
Mr_Fhqwhgads

Setting Chars

This topic is 4236 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I am trying to set a char(or string, if thats better) to somethign from the constructor.
	Cvar( const char *aName, const char *aValue, int aFlags ) 
	  {
		  strcpy( name, aName);
		  strcpy( sValue, aValue);
		  flags = flags;
	  }
The part where it does the strcpy throws a runtime error saying ti can't access memory. I have tried strings, changing it to/from pointers, not exacly sure whats wrong with it.
class Cvar
{
public:
	Cvar *next;
	char *name, *sValue;
	int flags;

	Cvar( const char *aName, const char *aValue, int aFlags ) 
	  {
		  strcpy( name, aName);
		  strcpy( sValue, aValue);
		  flags = flags;
	  }
	 //char *GetValue();
};

Share this post


Link to post
Share on other sites
Advertisement
Guest Anonymous Poster
You have horrid variable and class naming habits, unless you plan to work alone forever get into better habits...


class Cvar
{
public:
Cvar *next;
char *name, *sValue;
int flags;


Cvar( const char *aName, const char *aValue, int aFlags )
{
name = new[GetLength(aName)];
sValue = new[GetLength(aValue)];
strcpy( name, aName);
strcpy( sValue, aValue);
// This was flags = flags which was wrong...
flags = aFlags;
}
~Cvar()
{
delete [] name;
delete [] sValue;
}

//char *GetValue();
};


A better solution...


class Cvar
{
public:
Cvar *next;
std::string name, sValue;
int flags;

Cvar( const std::string aName, const std::string aValue, int aFlags )
{
name = aName;
sValue = aValue;
// This was flags = flags which was wrong...
flags = aFlags;
}
~Cvar()
{
}

//char *GetValue();
};



There, isn't that much better, cleaner, easier? :)

Share this post


Link to post
Share on other sites
aw I don't think the naming done is bad. I've seen (and sometimes use) structs with a member variable 'name' with a constructor that takes an argument 'theName', which isn't much different from using 'name' and 'aName' like in the example. I think it's more sane reading 'somestruct.name' instead of 'somestruct.m_name'.

(Although, I usually only do this though when the struct is only holding data, and doesn't have any real functionality other than that. For classes that actually do more complicated things I use the 'm_foo' convention for member variables to make it easier to understand the scope of variables in member functions.)

ANYWAYS! different strokes for different folks?

if you go with the std::string implementation you should use initialization lists too, so


Cvar( const std::string aName, const std::string aValue, int aFlags )
: name( aName )
, sValue( aValue )
, flags( aFlags )
{
}


Share this post


Link to post
Share on other sites
If you really want to take advantage of the STL,


struct ValueType
{
std::string Value; int Flags;

ValueType(const std::string &V="",int F=0) : Value(V),Flags(F) { }
};

typedef std::map<std::string,ValueType> ValueTable;

ValueTable Vars;

void f()
{
Vars["a"]=ValueType("hello",23);

cout << Vars["a"].Value;
}


would be my suggestion, although I'm second-guessing what you are trying to do a bit.

Share this post


Link to post
Share on other sites
some more refactoring :-)


// Close to your version (see below for a better version)
class Var
{
public:
MyClass *Next;
std::string Name;
std::string Value;
int Flags;

// fixed this to pass by reference, also why use aName or TheName
// when you can just use name? even if you used Name saying
// Name(Name) in the initializer list is still unambiguise
// and you can use this->Name in the body to resolve ambiguity
Var(const std::string& name, const std::string& value, int flags):
Name(name),
Value(value),
Flags(flags)
{
}
};

// This seems far more logical IMO but i had to assume a few things about
// what you were doing

class Data
{
public:
Var(const std::string& name, const std::string& value, int flags):
mName(name),
Value(value),
mFlags(flags)
{
}

const std::string& GetName();
void FlipFlag(int flagNum);
void SetFlag(int flagNum, bool value);
// do you ever logically need to get the flags?
// if so what about this instead
bool GetFlag(int flagNum);

public: // just personal preference to start a new section for variables
std::string Value; // should this really be a string maybe std::vector<char>

private:
const std::string mName; // Does it really make sense for this to be able to change?
int mFlags;
};

// also you probally noticed i got rid of your Cvar* next, thats because
// it seems to me like thats being used in a linked list style so just
// make a std::list<Cvar> when you want to use some Cvar's :-)



i dont think theres much more i can say without some context so bye

Edit: hmm i prefer what EasilyConfused (i missed that interpretation) said but im not sure that either that or what i said are what your trying to do.

Share this post


Link to post
Share on other sites
The reason it doesn't work, is that you're trying to strcpy() into something that isn't allocated yet. You should either just copy the pointer (Which is usually a bad idea, since the caller might have the buffer passed to the function on the stack), or to allocate memory in the class, either with new[] or by using a static buffer, like char sValue[256];.

Although, if this is C++, you should definitely go for the std::map of std::strings idea. It'll be faster, far less error prone, and easier to write and maintain.

Share this post


Link to post
Share on other sites
That "next" pointer makes me *very* suspicious. Are you building your own linked list? Don't do that; the standard library already provides a perfectly good one, std::list.

Although, if you're going to be doing all your lookup by searching for the "name", as EasilyConfused is guessing, then you instead want std::map. That is a data container that actually implements those semantics directly (lookup by a 'key'), and will provide faster searches than are possible with a linked list (because it internally sorts the data and can find the item by binary search; you *could* binary-search a doubly-linked list, but it isn't any faster than sequential search on a doubly-linked list).

And oh, yeah. std::string, and initializer lists. I don't know what language you've been using, but welcome to C++.

Basically, what EasilyConfused said. Except that you don't even need to define a "value type" for something this simple, because again, the standard library provides:


typedef std::pair<std::string, int> ValueType;
// That hard-wires the field names, because there's no way you could specify
// them via the template. .first replaces .Value; .second replaces .Flags .
// But it saves you all that class creation work :)

typedef std::map<std::string, ValueType> ValueTable;

ValueTable Vars;

void f() {
Vars["a"] = ValueType("hello", 23);
cout << Vars["a"].first;
}


There, isn't that much better, cleaner, easier? ;)

Share this post


Link to post
Share on other sites
Quote:
Original post by Evil Steve
The reason it doesn't work, is that you're trying to strcpy() into something that isn't allocated yet.
Finally, someone actually addressing the problem, and answering the question :)

Share this post


Link to post
Share on other sites
Quote:
Original post by Damon Shamkite
Quote:
Original post by Evil Steve
The reason it doesn't work, is that you're trying to strcpy() into something that isn't allocated yet.
Finally, someone actually addressing the problem, and answering the question :)


Um... the AP did that on the first reply. I'm not knocking Steve's clarification, of course, but I think once the problem has been answered in simple terms it is fair game to suggest improvements.

Share this post


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

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!