4
\$\begingroup\$

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"; 
}
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hi, Corbin. Your suggestion on how to make the MovieData function more efficient is clear but I am confused with the promptLine. \$\endgroup\$ – Duck Apr 21 '15 at 6:06
  • \$\begingroup\$ @Mary you mean how it works or what the purpose is? If it's the purpose: there's really not one other than to save a few lines and trim down on repetition. Even 2 lines can be painful to change if necessary in the future if they're repeated enough places. \$\endgroup\$ – Corbin Apr 21 '15 at 6:13
  • \$\begingroup\$ Really, it's pretty arbitrary in this case since the program is so small and the function does so little. It could be useful in a program or programs where line reading happens a lot more or where failure is more likely to happen though (like if you were reading lines from a file and wanted to handle only the exceptional case of a read failing instead of checking each read individually). It also gives a nice (subjective) consistency to have a line version to go along with the int version. \$\endgroup\$ – Corbin Apr 21 '15 at 6:14
  • \$\begingroup\$ I do get the purpose is to trim down on repetition and to save space. I am running it on Visual Studios and I keep getting an error even though I typed it in how you showed above as movie.title = promptLine("Enter the title of the movie: "); \$\endgroup\$ – Duck Apr 21 '15 at 6:16
  • \$\begingroup\$ @Mary Ah, I don't have a compiler handy at the moment. What error are you getting? I probably made a silly typo. Or, if you're using the promptLine("..."), you'll need to make a version of that like the promptInt("...") example. Edit: ah, yeah, you'll need to make a wrapper or modify it to use cin/cout instead of taking params. The wrapper is just std::string promptLine(const std::string& prompt) { return promptLine(std::cout, prompt, std::cin); } \$\endgroup\$ – Corbin Apr 21 '15 at 6:20
3
\$\begingroup\$

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(); 
\$\endgroup\$
  • \$\begingroup\$ Hello, Hosch250. You do bring up an interesting point about validating input. However, I don't want to necessarily create a whole new function. It would be troublesome to create it and insert it into the other function. If I were to validate the input step by step, how would I do it? Using variations of the cctype like isdigit and what not? \$\endgroup\$ – Duck Apr 21 '15 at 4:00
  • \$\begingroup\$ It actually won't be a big deal to insert it in the other function, let me post an example in the answer. \$\endgroup\$ – Hosch250 Apr 21 '15 at 4:01
  • \$\begingroup\$ You see, you can return values, and set variables equal to returned values and all sorts of cool things. If you didn't create a function, you would have to post that code in every place you needed to input an integer, but this way you can reuse your code, and only need to maintain it in one place. \$\endgroup\$ – Hosch250 Apr 21 '15 at 4:05
  • \$\begingroup\$ Given the structure of the input, it would be a lot simpler to read a line at a time and attempt to parse it as an int. Character by character parsing seems a bit over kill. \$\endgroup\$ – Corbin Apr 21 '15 at 4:08
  • \$\begingroup\$ @Corbin I actually tried that, but it has the same problem as the first version, where it only parses the first part of the string if it starts with an integer value. \$\endgroup\$ – Hosch250 Apr 21 '15 at 4:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.