Create a structure to store data about a movie

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.

• Title

• Director

• Year Released

• Running time (in minutes)

• Production cost

• 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
#include

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 > temp.year_released;

cout > temp.running_time;

cout > temp.production_cost;

cout > temp.firs

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
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.

StackExchange Code Review Q#87497, answer score: 6