Переопределение операторов, создание собственного класса String

[БалаШагаЛ]

Дали задание создать собственный класс String. Вот то, что написалось:

#include <iostream>
#include <stdio.h>
#include <string.h>
#include <conio.h>

class String
{
public:
	String();
	String(const char*);
	String(String&);
	~String();
	void SetString(const char*);
	void AddString(const char*);
	int GetLength();
	char *GetString();
	const String& operator+(String&);
	void operator+=(String&);
	//void operator=(String&);
private:
	char *t;
};

String::String()
{
	t=NULL;
}

String::String(const char *text)
{
	t=new char[strlen(text)];
	sprintf(t,"%s",text);
}

String::String(String &S)
{
	if (S.t)
	{
		t=new char[strlen(S.t)];
		sprintf(t,"%s",S.t);
	}
}

String::~String()
{
	if (t)
		delete [] t;
}

void String::SetString(const char *text)
{
	if (t)
		delete [] t;
	t=new char[strlen(text)];
	sprintf(t,"%s",text);
}

void String::AddString(const char *text)
{
	if (t)
		t=strcat(t,text);
	else
		this->SetString(text);
}

int String::GetLength()
{
	return strlen(t);
}

char *String::GetString()
{
	return t;
}

const String& String::operator+(String &S)
{
	String R(*this);
	R.AddString(S.GetString());
	return R;
}

void String::operator+=(String &S)
{
	this->AddString(S.GetString());
}

/*void String::operator=(String &S)
{
	this->SetString(S.GetString());
}*/

int main()
{
	String S1("aa");
	String S2;
	S2.SetString("bb");
	S2.AddString("cc");
	String S3;
	S3=S1+S2;
	S3+=S2;
	std::cout << S3.GetString();
	getch();
}

Всё работало хорошо до введения оператора =. После этого программа стала выбивать ошибку при отладке - мол, куча повреждена. Не вижу в чём ошибка, прошу помощи. Пишу в Visual Studio 2010.

[_Bers]

char *String::GetString()  //<---- выдает наружу сырой мутабельный указатель. Нарушение инкапсуляции, нарушение инварианта класса
{
	return t;
}

void String::operator=(String &S)
{
	this->SetString(S.GetString());  //<--- получает во владение сырой указатель, который пренадлежит ещё и другой сроке тоже
}

После этого, уже две строки нацелена на один и тот же указатель.
Далее, когда у первой строки закончится срок жизни, она сделает:

String::~String()
{
	if (t)
		delete [] t;
}

После чего данные по указателю уже не валидны. Но вторая строка об этом не знает. И когда закончится её срок жизни, то она тоже сделает:

String::~String()
{
	if (t)
		delete [] t;
}

Получается двойной делете одного и того же.

/зы: велосипед ужассен. Во первых он ни разу не соответствует с++ way
Класс не соответствует утиной нотации STL
Во-вторых, требует подключать файлы стандартной библиотеки.

В третьих, и это самый главный твой минус - нарушение инварианта класса.
Твой класс никак не оберегает собственные данные от повреждений дураком.

Отсутствие const-наци, излишки в интерфейсе сразу выдает дилетанта.

Например:

void String::operator+=(String &S)
{
	this->AddString(S.GetString());
}

Один из методов явно лишний. Или оставь оператор, или оставь AddString.
Классу не нужны методы, которые дублируют функционал друг друга.

Другой пример:

int String::GetLength()
{
	return strlen(t);
}

Я не буду докапываться до жутко тормозного способа узнать длину строки.
И даже промолчу, что проще вообще отказаться от такого класса, и напрямую пользовать си-библиотеку.
Совсем промолчу: а начерта мне такой класс, если он итак тянет за собой
#include<string.h>

Однако, ответь мне на вопрос: Почему твоя функция возвращает int?
У тебя что, длина строки может быть отрицательной???

Как длина строки может быть отрицательная?

Когда ты используешь чужой функционал, ты вообще смотришь, как с ним работать?

Ты использовал библиотечную:

size_t strlen ( const char * str );

Какой тип данных она возвращает? А какой ты возвращаешь?

Ты понимаешь, что ты приводишь беззнаковый тип к знаковому? Ты вообще головой думал, когда писал такое?


А это что такое?

String(String&);

Ты так организуешь конструктор копии?

Короче - сыро.

[Kostia]

t=new char[strlen(S.t)];
		sprintf(t,"%s",S.t);

Вы забыли про символ '\0', который определяет конец стоки.
sprintf - самый медленный и ужасный вариант копирования строк. Можно воспользоваться тем же memcpy и переписать SetString следующим образом:

void String::SetString(const char *text)
{
	if (t) delete [] t;
	t=NULL;
	size_t len = strlen(text);
	if (len > 0){
		t=new char[len+1];
		memcpy ( t, text, len+1 );
	}
}

так было бы правильнее:

const char *String::GetString() const
{
	return t;
}

В плане оптимизации, можно хранить приватную переменную указывающую длину строки и менять ее только тогда, когда меняется строка.

Длина строки:

n=0;
while(*(str + n)) n++;

Сложение строк:

t = new char[len1+len2+2];
memcpy(t, t1, len1+1);
memcpy(t+len1+1, t2, len2+1);

конструктор копирования:

String::String(const String &S)

Сообщение от _Bers

После этого, уже две строки нацелена на один и тот же указатель.

Нет, это не так. В SetString происходит копирование строки, хоть и с ошибкой. В остальном полностью согласен.

[Abstraction]

const String& String::operator+(String &S)
{
	String R(*this);
	R.AddString(S.GetString());
	return R;
}

Возвращается ссылка на локальный нестатический объект. Ошибка (объект перестаёт существовать в момент выхода из функции).
S.GetString может вернуть NULL, R.AddString(NULL) в его нынешней реализации приведёт к отказу приложения.

void String::operator=(String &S)
{
	this->SetString(S.GetString());
}

Подумайте, что будет, если S и this - один и тот же объект.

void String::AddString(const char *text)
{
	if (t)
		t=strcat(t,text);
	else
		this->SetString(text);
}

Плохая новость: strcat() дописывает text в массив t. Вопрос: сколько у Вас свободного места в массиве t? (Ответ: ноль).

Принято, чтобы операторы = и += возвращали константную ссылку на результат присвоения. Это позволяет писать, аналогично стандартным типам:

S1 = S2 = S3; //S1.operator=(S2.operator=(S3));