Output comes out wrong

Question

I am teaching myself C/C++ at the moment, and I got the exercise (from the book I am reading) to write a program that could make an output like this:

Enter your first name: Flip
Enter your last name: Fleming
Here’s the information in a single string: Fleming, Flip

Using Structures. But my output comes out like this:

Enter your first name: Flip
Enter your last name: Fleming
Here’s the information in a single string: , 

Here is the code. It's fairly short and simple so it shouldn't be hard to read :)

#include <iostream>
#include <cstring>

using namespace std;

struct Person {
    char* firstName;
    char* lastName;
};

char* getName(void);

int main() {
    Person* ps = new Person;
    cout << "Enter your first name: ";
    char* name;
    name = getName();
    ps->firstName = name;
    cout << "Enter your last name: ";
    char* lastname;
    lastname = getName();
    ps->lastName = lastname;
    cout << "Here's the information in a single string: "
            << ps->lastName << ", " << ps->firstName;
    delete ps;
    delete name;
    delete lastname;

    return 0;
}

char* getName() {
    char temp[100];
    cin >> temp;
    cin.getline(temp, 100);
    char* pn = new char[strlen(temp) + 1];
    strcpy(pn, temp);

    return pn;
}

Answer 1

First, there's no such thing as C/C++. You're mixing them, which is wrong. Since you're using C++ headers/new/using, I'll assume you want C++, so here's how you fix your code:

• replace all char* and char[] with std::string
• get rid of dynamic allocation

So, some changes would be:

struct Person {
    std::string firstName;
    std::string lastName;
};

or

Person ps;

Answer 2

You are using:

cin >> temp;
cin.getline(temp, 100);

You probably overwrite what you already have with empty string at the end of a line.

Use just one of them.

If you'll stick with using cin >> you may consider setting width() to prevent buffer overflow.

Answer 3

No no no, wayyyy too complicated. Use real C++ idioms. The program could be as simple as this:

#include <string>
#include <iostream>

int main()
{
    std::string firstName, lastName;

    if (!(std::cout << "Your first name: "  &&
          std::getline(std::cin, firstName) &&
          std::cout << "Your last name: "   &&
          std::getline(std::cin, lastName)     ))
    {
        std::cerr << "Error: unexpected end of input!\n";
        return 0;
    }

    std::cout << "You are " << firstName << " " << lastName << ".\n";
}

As a variation on the theme, you could put each getline in a loop until the user inputs a non-empty line:

std::cout >> "Your first name: ";
for ( ; ; )
{
    if (!(std::getline(std::cin, firstName))
    {
        std::cerr << "Error: unexpected end of input.\n"; 
        return 0;
    }
    if (!firstName.empty())
    {
        break;
    }
    std::cout << "Sorry, please repeat - your first name: ";
}

Answer 4

First, the immediate problem is that you read twice from std::cin: first with operator>>, and then with getline. Pick one or the other.

But let's simplify your code a bit. There are simply too many sources of error. Pointers are tricky because they might point to the wrong thing, or you might forget to delete objects, or delete them twice. C-style char arrays as strings are bad because, well, they're not strings, and they don't behave like strings.

So let's use the standard library's string class:

#include <iostream>
#include <string>

struct Person {
    std::string firstName;
    std::string lastName;
};

std::string getName(void);

int main() {
    Person ps;
    cout << "Enter your first name: ";
    std::string name = getName();
    ps.firstName = name;
    cout << "Enter your last name: ";
    std::string lastname = getName();
    ps.lastName = lastname;
    cout << "Here's the information in a single string: "
            << ps.lastName << ", " << ps.firstName;
}

std::string getName() {
    std::string temp;
    std::getline(cin, temp);
    return temp;
}

This is a fairly simple, almost mechanical substitution, basically just replacing char* by std::string, and removing the bits that are no longer necessary.

Of course, as pointed out in a comment, I've omitted all forms of error checking, which a real program should definitely do.