这是好的可靠代码吗? [英] Is this good, solid code?

问题描述

我已经设计了自己的字符串类，如果专家能够看一下，我将不胜感激.我花了昨天的大部分时间从头开始编写此文件，今天早上花了一个小时才删除内存泄漏.如果它在某种程度上类似于Standard Libray CString类，我想这意味着我处于正确的轨道.

头文件

I''ve designed my own string class and I would appreciate it if the experts would give a look at it. It took me most of yesterday afternoon to write this from scratch and an hour this morning removing memory leaks. If it resembles the Standard Libray CString class in any way, I guess it means I''m on the right track.

Header file

#pragma once

class cString
{
private:
	// Variables
	char * string;
	int cStringLength;
	static const int STR_MAX = 25500;
	static bool stringsJoined;

	// Encapsulated Methods
	int GetStringLength(char*) const;
	void CopyToLocalString(char*);
	void DeleteLocalString();
	bool CompareStrings(char*) const;
	char * JoinStrings(char*);

public:
	// Constructors & Deconstructor
	cString();
	cString(char*);
	virtual ~cString();

	// Operator Overloads
	void operator = (cString&);
	void operator = (char*);
	operator char*();
	bool operator == (cString&) const;
	bool operator == (char*) const;
	char * operator + (cString&);
	char * operator + (char*);
	void operator += (cString&);
	void operator += (char*);

	// Accessor methods
	char * GetString() const { return this->string; }
	int Size() const { return this->cStringLength; }
};

源代码

Source Code

#include "cString.h"

bool cString::stringsJoined = false;

cString::cString()
{
	this->string = 0;
}

cString::cString(char* String)
{
	this->string = 0;
	CopyToLocalString(String);
}

cString::~cString()
{
	DeleteLocalString();
}

int cString::GetStringLength(char * String) const
{
	// local variable
	int i;

	// Loop through the string until a null is found or
	// the max limit is reached
	for ( i = 0 ; i < STR_MAX ; i++ )
		if ( String[i] == 0 )
			break;

	// Return a -1 if the operation fails
	if ( i >= STR_MAX )
		return -1;
	// Return value of i if operation succeeds
	else
		return i;
}

void cString::CopyToLocalString(char * String)
{
	// Local variables
	int sourceLength;

	// Delete local string
	this->DeleteLocalString();

	// Once local string is cleared, begin copy operations.
	// -> Determine the length of the source string
	sourceLength = this->GetStringLength(String);
	// --> Ensure operation succeeded
	if ( sourceLength < 0 )
	{
		// If length of the source string failed the size check
		// create an empty string
		this->string = new char[1];
		this->string[0] = '\0';
		return;
	}

	// If length operation returned a valid value, continue
	// -> Set the cStringLength field to sourceLength + 1
	this->cStringLength = sourceLength + 1;
	// -> Create a new char array
	this->string = new char[this->cStringLength];
	// -> Copy the source string into the new array
	for ( int i = 0 ; i < this->cStringLength ; i++ )
		this->string[i] = String[i];
	// -> Null terminate the new string.
	this->string[this->cStringLength - 1] = '\0';
}

/*  cString Method : DeleteLocalString()
	Checks to see if data is stored behind the string pointer. If data
	is found, the char array is deleted and the pointer nullified.
*/
void cString::DeleteLocalString()
{
	if (this->string != 0)
	{
		delete this->string;
		this->string = 0;
	}
}

/*  cString Method : CompareStrings(char*)
	Compares the string in the char* pointer to the string
	currently held in the local string variable
*/
bool cString::CompareStrings(char * String) const
{
	// Local variables
	int compareStringLength = GetStringLength(String) + 1;
	int comparisonTotal;

	// Compare lengths
	comparisonTotal = ( cStringLength - compareStringLength );
	if ( comparisonTotal != 0 )
		return false;

	// If strings are equal lengths, begin character by
	// character comparison
	for ( int i = 0 ; i < cStringLength ; i++ )
	{
		comparisonTotal = ( this->string[i] - String[i] );
		// If the strings are different, total will != 0;
		if ( comparisonTotal != 0 )
			return false;
	}

	return true;
}

/*  cString Method : JoinStrings(char*)
	Concaternates the characters int char* parameter with the
	characters currently stored in the local string
*/
char * cString::JoinStrings(char* String)
{
	// Local variables
	int totalLength = this->cStringLength + ( GetStringLength(String) );
	int placeHolder = 0;

	// Create a new char array that will fit both strings + null
	char * strJoin = new char[totalLength];

	// Copy first string into array
	for ( placeHolder ; placeHolder < this->cStringLength ; placeHolder++ )
		strJoin[placeHolder] = this->string[placeHolder];

	placeHolder--;

	// Copy second string into array
	for (int i = 0 ; placeHolder < totalLength ; i++, placeHolder++ )
		strJoin[placeHolder] = String[i];

	// Add null terminator
	strJoin[totalLength - 1] = '\0';

	// Return the finished string
	return strJoin;
}

/* cString Operator Assignment Overloads */
// cString to cString copy
void cString::operator = (cString& cstring)
{
	if ( this == &cstring)
		return;
	else
		CopyToLocalString(cstring.string);
}
// char array to cString copy
void cString::operator = (char * string)
{
	if ( stringsJoined == true)
	{
		CopyToLocalString(string);
		delete string;	// Removes memory leak in joining operations
		string = 0;
		stringsJoined = false;
	}
	else
		CopyToLocalString(string);
}
cString::operator char*()
{
	return this->string;
}
/* cString comparison overloads */
// compare cString to cString
bool cString::operator == (cString & cstring) const
{
	if ( this == &cstring)
		return true;
	else
		return this->CompareStrings(cstring.GetString());
}
// compare cString to char string
bool cString::operator == (char * string) const
{
	return this->CompareStrings(string);
}
/* cString addition overloads */
// return a joined string, cString to cString
char * cString::operator + (cString & cstring)
{
	stringsJoined = true;
	return JoinStrings(cstring.GetString());
}
// return a joined string, cString to char string
char * cString::operator + (char * string)
{
	stringsJoined = true;
	return JoinStrings(string);
}
/* cString add then replace overloads */
// join cString to another cString
void cString::operator += (cString & cstring)
{
	char * tempString = this->JoinStrings(cstring.GetString());
	this->CopyToLocalString(tempString);
	delete tempString;
	tempString = 0;
}
// join cString to char string
void cString::operator += (char * string)
{
	char * tempString = this->JoinStrings(string);
	this->CopyToLocalString(tempString);
	delete tempString;
	tempString = 0;
}

我自己的测试表明，该类按预期工作.我只是在寻找稳定性或改进方面的投入.另外，评论是否还不错还是我过分了?

My own testing shows that this class acts as I intended. I''m just looking for input for stability or improvements. Also, is the commenting decent or did I overdo it?

推荐答案

该代码存在很多问题.

我的第一条建议是，如果您想编写良好的坚实代码，那就是阅读诸如以下的好书:

-有效的C ++
-更有效的C ++
-有效的STL
-卓越的C ++

在那些书中，您将学到很多东西，并有更多最常见的错误的解释，如代码中的错误.

话虽如此，这是对代码的一些注释.我可能会把它分成几个解决方案，否则可能会很长...

There is a lot of problems with that code.

My first recommandation if you want to write good solid code, would be to read good books like:

- Effective C++
- More Effective C++
- Effective STL
- Exceptionnal C++

In those book, you will learn a lot and have explaination more most common mistake like those in your code.

Having said that, Here is some comment for the code. I might split that in a few solution as otherwise it could be very long...

class cString
{
private:
    // Variables
    char * string;
    int cStringLength;

好吧，变量名string是可区分的，因为它像std::string一样被广泛用作类名，因此对人们来说可能有些混乱.

Well variable name string is discutable as it is widely used as a class name as in std::string so it be might be a bit confusing for people.

static const int STR_MAX = 25500;

您真的要为字符串类增加最大长度吗?对于通用类，它应该能够处理系统可以处理的所有内容.

Do you really want to put a maximum length to your string class? For a general purpose class it should be able to handle anything the system can handle.

static bool stringsJoined;

如果您有许多cString对象实例并在许多对象上调用operator +，则使用静态字段将无法正常工作.

Using a static field won''t works properly if you have many cString object instance and call the operator + on many objects.

// Encapsulated Methods
int GetStringLength(char*) const;

该方法应该是静态的(它不访问实例数据)，或者应删除参数，然后该函数对实例数据起作用.鉴于使用了该功能，第一种选择将是您所用的正确选择.

另外，该参数应为const:int GetStringLength(const char*) const;

另外，即使在声明中也要给参数命名.它将更好地记录您的代码，并且一些编辑器还可以在编辑代码时显示参数名称.

Either that method should be static (it does not access instance data) or the argument should be removed and the function works on the instance data. Given the use of that function, the first option would be the correct one in your case.

Also the parameter should be const: int GetStringLength(const char*) const;

Also, it is recopmmanded that you give name to your parameters even in declaration. It will document your code better and some editor will also be able display the parameter name while editing code.

void CopyToLocalString(char*);

参数应为常量

Parameter should be constant

void DeleteLocalString();

暂无评论

No comment

bool CompareStrings(char*) const;
char * JoinStrings(char*);

参数应为常数.第二个函数也应该是常量，因为不应使用此方法修改对象.

Parameters should be constant. The second function should also be constant as the object should not be modified by this method.

public:
    // Constructors & Deconstructor
    cString();
    cString(char*);
    virtual ~cString();

    // Operator will be discuted in a subsequent solution...
};

第二个构造函数的参数应为常数.还应该使该构造函数显式以避免静默转换:explicit cString(const char *source);

通常，对于像这样的类，析构函数将不会是虚拟的，因为通常我们不应从此类中派生出该析构函数，并且通过不进行虚拟化，它将有助于提高性能，并且使用的内存可能会少一些(没有vtable可以保留) .

复制构造函数丢失.在这种情况下，默认生成的副本构造函数不合适.如果复制了一个对象，则2个对象将引用相同的字符串，这将导致损坏.

假设原始字符串已经过验证，则实现可能如下所示:

The parameter for the second constructor should be constant. That constructor should also be made explicit to avoid silent convertion: explicit cString(const char *source);

Generally for a class like this one, the destructor would not be virtual as normally we should not derive from such a class and by not being virtual, it will help performance a bit and uses might use a bit less memory (no vtable to keep).

Copy constructor is missing. Default generated copy constructor is not appropriate in this case. If an object is copied, then 2 object would reference the same string which will lead to corruption.

Assuming that the original string has already been validated, the implementation might look like:

cString::cString(const cString &other) :
    cStringLength(other.cStringLength)
{
    // Since the original string has been validated and contains
    // a final '\0', this function can easily be optimized a bit...

    string = new char[cStringLength];

    // Copy could be done with memset which is more efficient
    // but for educationnal purpose, I use a loop
    for (int i = 0; i != cStringLength; ++i)
    {
        string[i] = other.string[i];
    }
}

现在可用于运算符...

Now for operators...

// Operator Overloads
void operator = (cString&);
void operator = (char*);
operator char*();
bool operator == (cString&) const;
bool operator == (char*) const;
char * operator + (cString&);
char * operator + (char*);
void operator += (cString&);
void operator += (char*);

您的操作员应遵循标准"原型.主要原因是为了与内置类型保持一致.例如，内置类型允许链接=.即:

Your operators should follow "standard" prototypes. The main reason is for consistency with built-in types. For example, built-in type allows chaining of =. That is:

int a, b, c;
a = b = c = 5;

因此，您应该具有:

Thus you should have:

// Operator Overloads
cString & operator = (const cString&);
cString & operator = (const char*);

bool operator == (const cString&) const;
bool operator == (const char*) const;

cString operator + (const cString&);
cString operator + (const char*);

cString& operator += (const cString&);
cString& operator += (const char*);

运算符==和运算符+应该是全局(朋友)函数，并且在左边的对象是const char *而右边的对象是cString的情况下，应该有另一个函数.

operator char*();
您应该避免为此类使用转换运算符.尽管这似乎使您的课程更易于使用，但它带有许多陷阱.这就是为什么像std::string这样的类没有该类，而是提供一个显式函数c_str()以便在需要时获取内部字符串的原因.
您的GetString函数就是这样做的.但是也最好返回const char *以避免外部修改.例如，您不希望有人在当前结尾之前写"\ 0"来弄乱长度.

Operator == and operator + should be global (friend) functions and there should be another function for the case where the object on the left is a const char * and the one on the right is a cString.

operator char*();
You should avoid having a conversion operator for such class. Although is seem to make your class easier to use, it come with many pitfalls. This is the reason that class like std::string do not have it and instead provide an explicit function c_str() to get the internal string when required.

Your GetString function just do that. But it is also preferable to return a const char * to avoid external modifications. For example, you don''t want your length to be wrong by someone writing a ''\0'' before its current end.

对于实现，还有很多事情要做可以说...我会尽量简短，并简单地解释主要问题.

For the implementation, there is also a lot to be said... I will try to be brief and simply explain main problems.

cString::cString()

该构造函数将字符串设置为0，但是其余大部分代码不会检查这种情况.

在这种情况下，cStringLength也不会初始化.

That constructor set string to 0 but most of the rest of the code does not check for that case.

Also cStringLength is not initialized in that case.

int cString::GetStringLength(char * String) const

此函数的问题在于它在错误时返回-1，但是客户端代码并不总是对其进行检查.例如，JoinStrings不能正确处理这种情况.

The problem with this function is that it returns -1 on error but the client code does not always check it. For example JoinStrings does not properly handle that case.

void cString::CopyToLocalString(char * String)

此功能并非异常安全.如果引发异常(内存不足)，则字符串成员将指向null.

This function is not exception safe. If an exception is thrown (out of memory), then the string member would point to null.

bool cString::CompareStrings(char * String) const

由于此函数有2个循环，因此效率可能会降低.

Since this function has 2 loops, it might be less efficient than it could be.

void cString::operator = (char * string)

此功能显然是错误的...应该是这样的:

This function is plainly wrong... It should be somthing like:

cString& operator=(const char *source)
{
    CopyToLocalString(source);  // Assume that the function is first fixed...
    return *this;
}

实际上，对于两个赋值运算符，使用swap技巧甚至会更好:

更多C ++习语-复制和交换 [分配运算符 [使用交换使赋值运算符异常安全 [ ^ ]

GotW#59 [

And in fact, using the swap trick would even be better for both assignment operators:

More C++ Idioms - Copy and swap[^]

Assignment operator[^]

Using swap to make assignment operator exception safe[^]

GotW #59[^]

Your + and += operators could be improved either for efficiency or for correctness or both. As an example both+= operators make a useless copy of the string as JoinStrings as already make a copy.

这篇关于这是好的可靠代码吗?的文章就介绍到这了，希望我们推荐的答案对大家有所帮助，也希望大家多多支持IT屋！