Revisions to Create a structure to store data about a movie

replaced http://stackoverflow.com/ with https://stackoverflow.com/

Source Link

edited May 23, 2017 at 12:40

Community Bot

1

Using using namespace std; is a bad habit bad habit to get into.
Overusing std::endl can likewise be a bad habit 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 bad habit

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

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

added 819 characters in body

Source Link

edited Apr 21, 2015 at 5:00

Corbin

10.6k
2
30
51

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.

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.

Source Link

created Apr 21, 2015 at 4:54

Corbin

10.6k
2
30
51

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

Stack Exchange Network

Return to Answer