How does get line work?

Question

I am trying to write my own getline function and it keeps on segfaulting. How do I fix it and how does get line work officially if mine is not functional? I am writing this to learn how to code better.

#include"MyString.h"





MyString::MyString( )  //constructor
{
    size=0;
    capacity=1;
    data=new char[capacity];

}
MyString::MyString(char * n)  //copy constructor
{
    size=strlen(n);
    capacity=strlen(n)+1;
    data=new char[capacity];
    strcpy(data,n);
}
MyString::MyString(const MyString &right)  //
{
    size=strlen(right.data);
    capacity=strlen(right.data)+1;
    data=new char [capacity];
    strcpy(data,right.data);

}
MyString::~MyString( )
{
    delete [] data;
}
MyString  MyString::operator = (const MyString& s)
{

    if(this!=&s)
    {
        MyString temp=data;
        delete [] data;
        size=strlen(s.data);
        capacity=size+1;
        data= new char [capacity];
        strcpy(data,s.data);
    }
}
MyString&  MyString::append(const MyString& s)
{
    if(this!=&s)
    {
        strcat(data,s.data);
    }


}
MyString&  MyString::erase()
{

}
MyString  MyString::operator + (const MyString& s)const
{
    return strcat(data,s.data);
}
bool  MyString::operator == (const MyString& s)
{
    return strcmp(data,s.data)==0;
}
bool  MyString::operator <  (const MyString& s)
{
    return strcmp(data,s.data)<0;
}
bool  MyString::operator >  (const MyString& s)
{
    return strcmp(data,s.data)>0;
}
bool  MyString::operator <= (const MyString& s)
{
    return strcmp(data,s.data)<=0;
}
bool  MyString::operator >= (const MyString& s)
{
    return strcmp(data,s.data)>=0;
}
bool  MyString::operator != (const MyString& s)
{
    return strcmp(data,s.data)!=0;
}
void  MyString::operator += (const MyString& s)
{
    append(s.data);
}
char&  MyString::operator [ ] (int n)
{
    return data[n];
}
void  MyString::getline(istream& in)
{
    char c;
    erase();
    ifstream input;
    while(in.get(c)&&c!='\n')
    {
        data[size]=c;
        size++;

        if(size+1<=capacity)
        {
          capacity*=2;
          char*p=new char[capacity];
          strcpy(p,data);
          delete [] data;
          data=p;
        }
        data[size]=c;
        size++;
        data[size]='\0';
    }

}
int  MyString::length( ) const
{
    return strlen(data);
}
void MyString::grow()
{
 capacity=strlen(data)+1;
MyString temp;
temp=data;
delete [] data;
capacity*=2;
data= new char[capacity];
}

ostream& operator<<(ostream& out, MyString& s)
{

    out<<s.data;
    return out;


}



// int  MyString::getCapacity(){return capacity;}

Answer 1

MyString::MyString(char * n)  //copy constructor
{
    size=strlen(n);
    capacity=strlen(n)+1;
    data=new char[capacity];
    strcpy(data,n);
}

There's no point calling strlen twice. Optimizers aren't smart enough to eliminate the redundancy. Also, since you aren't altering the data in the passed-in string, it should be const. Otherwise you couldn't pass it string constants. Fixing, we get:

MyString::MyString(const char * n)  //copy constructor
{
    size = strlen(n);
    capacity = size + 1;
    data = new char[capacity];
    strcpy(data, n);
}

---

MyString::MyString(const MyString &right)  //
{
    size=strlen(right.data);
    capacity=strlen(right.data)+1;
    data=new char [capacity];
    strcpy(data,right.data);

}

right has a size member that eliminates the need for strlen. That plus the fixes above give:

MyString::MyString(const MyString &right)  //
{
    size = right.size;
    capacity = size + 1;
    data = new char[capacity];
    strcpy(data, right.data);
}

---

MyString  MyString::operator = (const MyString& s)
{

    if(this!=&s)
    {
        MyString temp=data;
        delete [] data;
        size=strlen(s.data);
        capacity=size+1;
        data= new char [capacity];
        strcpy(data,s.data);
    }
}

operator = should return a reference (i.e. *this; BTW, you forgot to return anything), not another copy. Also note that temp is not used, and you have a redundant strlen call:

MyString& MyString::operator = (const MyString& s)
{
    if(this!=&s)
    {
        delete[] data;
        size = s.size;
        capacity = size + 1;
        data = new char[capacity];
        strcpy(data, s.data);
    }
    return *this;
}

---

MyString&  MyString::append(const MyString& s)
{
    if(this!=&s)
    {
        strcat(data,s.data);
    }


}

You forgot to check your size and capacity. So strcat will run right off the end of your char array. Also, concatenation to itself should be made to work. And you don't need strcat:

MyString& MyString::append(const MyString& s)
{
    size_t rSize = s.size;
    if(capacity < size + rSize + 1)
    {
        capacity = size + rSize + 1;
        char* newData = new char[capacity];
        memcpy(newData, data, size);
        delete[] data;
        data = newData;
    }
    memcpy(data + size, s.data, rSize);
    size += rSize;
    data[size] = '\0';
    return *this;
}

---

erase just needs to make the string go away. No memory needs to be messed with:

MyString& MyString::erase()
{
    size = 0;
    data[0] = '\0';
}

---

MyString  MyString::operator + (const MyString& s)const
{
    return strcat(data,s.data);
}

Same problems as with append, plus this one should return a new object and leave its inputs alone. Also, reuse the methods that have been written so far:

MyString MyString::operator + (const MyString& s) const
{
    return MyString(*this).append(s);
}

---

void  MyString::operator += (const MyString& s)
{
    append(s.data);
}

This is an insidious one! See, since append takes a const MyString& but you're giving it a char*, the compiler creates a temporary MyString, calls the MyString(const char*) constructor on it, passes the temporary to append, then discards it. So you get the right result but you created an extra object. The right way is:

void MyString::operator += (const MyString& s)
{
    append(s);
}

---

char&  MyString::operator [ ] (int n)
{
    return data[n];
}

No bounds checking? Brave. If you want it, it's not hard to add. Assuming you don't want to throw an exception:

char& MyString::operator [] (int n)
{
    if(n < 0)
        n = 0;
    if(n >= size)
        n = size - 1;
    return data[n];
}

FYI, the usual type for the index is size_t, which is defined in <stddef.h> and is the unsigned integer type returned by sizeof(), so it's guaranteed to work as an array index with a normal array (here, you could get rid of the negative index check).

Also, you might want a version that can work on a const MyString. It just returns the character instead of a reference:

char MyString::operator [] (int n) const
{
    if(n < 0)
        n = 0;
    if(n >= size)
        n = size - 1;
    return data[n];
}

---

void  MyString::getline(istream& in)
{
    char c;
    erase();
    ifstream input;
    while(in.get(c)&&c!='\n')
    {
        data[size]=c;
        size++;

        if(size+1<=capacity)
        {
          capacity*=2;
          char*p=new char[capacity];
          strcpy(p,data);
          delete [] data;
          data=p;
        }
        data[size]=c;
        size++;
        data[size]='\0';
    }

}

First, the ifstream does nothing; get rid of it. You should only work with the istream. indiv pointed out the problems with the extra characters and the bad check. The strcpy will fail because you haven't put the \0 back on the end of the string, so you need to use memcpy instead:

void  MyString::getline(istream& in)
{
    char c;
    erase();
    while(in.get(c) && c != '\n')
    {
        data[size++] = c;
        if(size >= capacity)
        {
          capacity *= 2;
          char *newData = new char[capacity];
          memcpy(newData, data, size);
          delete[] data;
          data = newData;
        }
    }
    data[size] = '\0';
}

---

int  MyString::length( ) const
{
    return strlen(data);
}

This is just wasting time because size already has the length of the string. It really should be defined in the class definition so it can be inlined:

int MyString::length() const
{
    return size;
}

---

void MyString::grow()
{
 capacity=strlen(data)+1;
MyString temp;
temp=data;
delete [] data;
capacity*=2;
data= new char[capacity];
}

The first line is simply wrong; capacity is already set right. You don't need temp. And yu forgot to copy the string data over:

void MyString::grow()
{
    capacity *= 2;
    char* newData = new char[capacity];
    memcpy(newData, data, size + 1);
    delete[] data;
    data = newData;
}

---

ostream& operator<<(ostream& out, MyString& s)
{

    out<<s.data;
    return out;


}

You're not changing s, so it should be const:

ostream& operator<<(ostream& out, const MyString& s)
{
    return out << s.data;
}

Answer 2

It should look something like this:

void MyString::getline(istream& in)
{
    erase();
    for (char c; in.get(c) && c != '\n'; )
        append(c);
}

Now you just need to properly implement a method called append that appends a single character. If you have trouble with that, then ask another question. You may think I'm being facetious here, but I am not. You need to limit the places that you reallocate and stop repeating yourself in your code. A getline function is not the place for that sort of activity (reallocations, I mean).

Answer 3

Well...

if(size+1<=capacity)

Let's say your capacity is 11 and your size is 11.

if( 12 <= 11 )
{
   // Resize capacity.  This code won't run.
}

You want if( size >= capacity ).

Also, you have data[size] = c; size++; twice in your loop. So you're making 2 copies of every character.