Create a structure to store data about a movie

Question

Below are the instructions for my program and the source code. My program runs and works but I would like second opinions on how to make my program more efficient or if there are any loose ends in my source code.

Using functional decomposition, write a C++ program that will use a structure called MovieData to store the following information.

1. Title
2. Director
3. Year Released
4. Running time (in minutes)
5. Production cost
6. First Year Revenue.

Then use a value returning function called getMovieData() to read data to each component of a struct variable as stated above. This function must return a variable of type MovieData.

Also use a void function called printMovieData() to print each member of struct in a nice format (with appropriate description). This function will accept a pointer to type MovieData, that is, its prototype will be like this: void printMovieData(MovieData *);

In your main() program, declare two variable of type MovieData with the following declaration: MovieData m1,m2;

And then call getMovieData() to assign value to each of m1 and m2.

Finally call printMovieData() on each of m1 and m2 to print values in each struct.

#include "stdafx.h" 
#include <iostream> 
#include <string> 

using namespace std; 

struct MovieData
{
    string title; 
    string director; 
    int year_released; 
    int running_time; 
    int production_cost; 
    int first_year_revenue; 
}; 

// Function prototypes
MovieData getMovieData(); 
void printMovieData(MovieData *); 

int main()
{
    // Variables 
    MovieData m1, m2; 
    MovieData *ptr1, *ptr2; 
    ptr1 = &m1; 
    ptr2 = &m2; 

    // Call getMovieData function to get information for both movies 
    m1 = getMovieData(); 

    m2 = getMovieData(); 

    // Call printMoviedata function to print information for both movies
    printMovieData(ptr1); 

    cout << "\n"; 

    printMovieData(ptr2); 

    system("PAUSE"); 

    return 0; 
}

/**
* Pre-Condition: 
* Post-Condition: 
*/
MovieData getMovieData()
{
    // Variables
    MovieData temp; 

    // Prompt user for information
    cout << "Enter the title of the movie: "; 
    getline(cin, temp.title); 

    cout << "\n"; 

    cout << "Enter the name of the movie's director: "; 
    getline(cin, temp.director); 

    cout << "\n"; 

    cout << "Enter the year the movie was released: "; 
    cin >> temp.year_released; 

    cout << "\n"; 

    cout << "Enter the running time of the movie in minutes: "; 
    cin >> temp.running_time; 

    cout << "\n"; 

    cout << "Enter the production cost of the movie: $"; 
    cin >> temp.production_cost; 

    cout << "\n"; 

    cout << "Enter the movie's first year revenue: $"; 
    cin >> temp.first_year_revenue; 

    cin.ignore(); 

    cout << "\n"; 

    return temp; 
}

/**
* Pre-Condition: 
* Post-Condition: 
*/
void printMovieData(MovieData *pointer)
{
    cout << "Title: " << pointer->title << endl; 
    cout << "Director: " << pointer->director << endl;
    cout << "Year Released: " << pointer->year_released << endl;
    cout << "Running Time: " << pointer->running_time << endl;
    cout << "Production Cost: " << pointer->production_cost << endl;
    cout << "First Year Revenue: " << pointer->first_year_revenue << endl;
    cout << "\n"; 
}

Answer 1

Looks pretty good overall. The biggest issue that I see is that the input reading doesn't check for errors, and it's pretty rough. It can be helpful with assignments like this to build up a little library of IO helpers:

// Read a line from the provided stream.
std::string readLine(std::istream& is) {
    std::string line;
    if (!std::getline(is, line)) {
        // Could stand to be a more specific exception, but you get the point
        throw runtime_error("Unexpected stream failure");            
    }
    return line;
}

// Helper to output a prompt and read a line.
std::string promptLine(std::ostream& os, const std::string& prompt, std::istream& is) {
    os << prompt; // Note that I've assumed output won't fail. Depending on situation,
                  // that might be a bad assumption.
    return readLine(is);
}

// Attempt to parse an int from a line of input that must contain only an int.
bool readIntLine(std::istream& is, int& val) {
    std::string line = readLine();
    char* parseEnd = nullptr; // NULL if < C++11
    int val = std::strtol(line.c_str(), &parseEnd, 10);
    return (parseEnd - line.c_str() == line.size());
}

// Keep showing the same prompt until the user inputs an integer (and only an integer)
int promptInt(std::ostream& os, const std::string& prompt, std::istream& is) {
    int val;
    do {
        os << prompt;
    } while (!readIntLine(is, val));
    return val;
}

// If you wanted, you could take it a step farther and make convenience wrappers that assume std::cin/std::cout.
// Example:    
int promptInt(const std::string& prompt) {
    return promptInt(std::cout, prompt, std::cin);
}

This looks like quite a bit of code (and it is -- unfortuntely IO just kind of sucks to do non-carelessly), but it makes reading in the data a bit cleaner while also providing verification of data:

MovieData getMovieData()
{
    MovieData movie; 

    movie.title = promptLine("Enter the title of the movie: ");
    movie.directory = promptLine("Enter the name of the movie's director: ")
    movie.year_released = promptLineInt("Enter the year the movie was released: ");
    // ...

    return movie; 
}

---

Other minor issues:

• Using using namespace std; is a bad habit to get into.
• Overusing std::endl can likewise be a bad habit as it not only writes a new line but also flushes the buffer. This can result in surprisingly bad performance when heavvy IO is involved. It's much better to use '\n' by default and only use std::endl when you specifically want a flush to happen (for example, your print function could use all \n and then have one std::endl at the end).
• Some of your comments are a bit pointless and should be removed to reduce noise.
• system("PAUSE") is a bad habit

---

As an aside, it might be worth noting that there's actually a super useful pattern of reading things from a stream. It's relatively poor performance considered to something specialized like std::strtol, but it can be quite handy if you're in a bind (i.e. can't use Boost or some other library) and need something highly generic:

template<typename T>
bool readLine(std::istream& is, T& val) {
    std::string line;
    if (!std::getline(is, line)) {
        // throw exception ...
    }
    std::istringstream ss(line);
    return (ss >> val);
}

It also might be worth noting that instead of a bool, functions like this will often return the input stream with certain bits set. In other words, some people prefer to set the fail bit instead of return false.

Answer 2

If you enter an invalid number for your int values, your program breaks. To prevent this, you could check your input like this:

int val = -1;

do
{
    if (!std::cin) {
        std::cin.clear();
        std::cin.ignore();
    }

    std::cin >> val;
} while (!std::cin);

This will make your user enter a value until a valid entry is entered. However, this will only work when the value entered is in the form iiisss, where i is an integer and s is a non-integer value. If the user enters 123.345, 123r, or 132+134 it will only input the 123 part and leave the other part in the cin stream.

To validate against these values, you can test the input more rigorously:

int val = -1;

while (true)
{
    bool isValidInput = true;
    string num = "";
    std::cin >> num;

    for (char c : num) {
        if (!isdigit(c)) {
            isValidInput = false;
            break;
        }
    }

    if (!isValidInput) {
        continue;
    }

    val = stoi(num);
    break;
}

This will input the entire string of input, make sure it contains only integer values, then parse the string as an integer and assign the value to the number. Unfortunately, this also breaks if you enter a value larger than an integer. You could manually check this before you attempt to convert the string input, or you could simplify your method a little and just use a try/catch block:

while (true)
{
    std::string num = "";
    std::cin >> num;

    for (char c : num) {
        if (!isdigit(c)) {
            num = "Bad Value";
            break;
        }
    }

    try {
        val = stoi(num);
        break;
    }
    catch (std::exception e) {
        std::cerr << e.what() << std::endl;
    }
}

Either way, I would recommend you make this a function of its own.

getIntInput()
{
    int val = -1;

    while (true)
    {
        std::string num = "";
        std::cin >> num;
        for (char c : num) {
            if (!isdigit(c)) {
                num = "Bad Value";
                break;
            }
        }

        try {
            val = stoi(num);
            return val;
        }
        catch (std::exception e) {
            std::cerr << e.what() << std::endl;
        }
    }
}

You could get your input like this then:

cout << "Enter the running time of the movie in minutes: "; 
temp.running_time = getIntInput();