0
\$\begingroup\$

I'm taking a course and I've just completed a challenge. The program runs well and it does everything I told it to, but I would like suggestions as to what can I implement to make it better (ex. pointers, move constructors etc.).

#include <iostream>
#include <vector>
#include <string>

using namespace std;

class Movie
{
private:
    friend class Movies;
    string name;
    string rating;
    int times_watched;
public:
    Movie(string name1, string rating1, int times_watched1)
        :name{ name1 }, rating{ rating1 }, times_watched{ times_watched1 } {}
};
class Movies
{
private:
    vector <Movie> list;
public:
    bool check_movie(const Movie &movie1)
    {
        for (int i = 0; i < list.size(); i++)
            if (list.at(i).name == movie1.name)
                return true;
        return false;
    }
    void add_movie(const Movie movie1)
    {
        if (check_movie(movie1) == false)
            list.push_back(movie1);
        else cout << "Movie is already in the list\n";
    }
    void increment_watched_count(Movie &movie1)
    {
        if (check_movie(movie1) == true)
        {
            for (int i = 0; i < list.size(); i++)
                if (list.at(i).name == movie1.name)
                    list.at(i).times_watched++;
        }
        else cout << "Movie is not on the list\n";
    }
    void display_list()
    {
        for (int i = 0; i < list.size(); i++)
            cout << list.at(i).name << "  |  " << list.at(i).rating << "  |  "
            << list.at(i).times_watched << endl;
    }
};

int main()
{
    Movies collection;
    Movie Star_Wars{ "Star Wars", "E", 100 }, Anna_Karenina{ "Anna Karenina","PG-13",100 };
    collection.add_movie(Movie{ "Dictator","PG-13",3 });
    collection.add_movie(Movie{ "Harry Potter","E",1000 });
    collection.add_movie(Star_Wars);
    collection.add_movie(Anna_Karenina);
    collection.display_list();
    collection.increment_watched_count(Anna_Karenina);
    collection.display_list();
    system("pause");
    return 0;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ If this is a programming challenge, please quote the text of the challenge itself in the question, add a link to the programming challenge and provided any example input and output. The title of the question should probably be the title of the programming challenge. We need more information to provide a good code review. Please read How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    Nov 11 at 13:44
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Nov 11 at 13:46
  • \$\begingroup\$ The program […] does everything I told it to Have the program tell everybody (including maintenance programmers and reviewers) what it exists to do. \$\endgroup\$
    – greybeard
    Nov 14 at 17:54
  • \$\begingroup\$ (what can I implement to make [my program] more efficient? Tests for continued usefulness, if not correctness. Instrumentation to have a solid base to verify and hunt down efficiency problems - why assume there was one?) \$\endgroup\$
    – greybeard
    Nov 14 at 17:57
2
\$\begingroup\$

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)


Movie(string name1, string rating1, int times_watched1)
        :name{ name1 }, rating{ rating1 }, times_watched{ times_watched1 } {}

You are passing the string arguments by value but you are not making use of the sink idiom. I don't know if you made the common mistake of not passing strings by reference (if you came from a language that had all objects be references), or were trying to get fancy and didn't get it right.


void add_movie(const Movie movie1)

OK, definitely the former. Print this out on a 4x6 inch card and tape it above your monitor:

C++ has value semantics


Here, you are telling it to make a complete copy of the entire Movie object, duplicating all the strings it contains.


 void increment_watched_count(Movie &movie1)

Here, you got the & but forgot the const. If you had used const on your data in main, you would have found out!


The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.


if (check_movie(movie1) == true)
if (check_movie(movie1) == false)

Don't write explicit tests against true or false. The result of check_movie is already a boolean value. So is the result of operator==. So if you want to go that way, you should write (check_movie(movie1)==true)==true) but now you still have a boolean result; you going to check that to be true too? It would never end!

The second one should be if (!check_movie(movie1)).


 for (int i = 0; i < list.size(); i++)

Use a range-based for loop. Write: for (const auto& m : list) {
and you do not need to do any subscripting. The variable m is available inside the body of the loop ready to use.


check_movie is just doing a search. Don't write code to search a vector from scratch. Use std::find or other standard algorithms. Wose, you duplicate the search code, rather than calling a common function to do this.

You might want to pass in a string only, rather than an entire Movie object, to be matched.

Here, you always use the name as the key. So maybe you just want to use a std::map instead?

The display should not be part of the class. Other code may want to do different things, and displaying to too specific. Rather, provide a general way to go through all the objects. Making the Movie object unusable outside of the list class hinders simply exposing the collection in a standard manner

\$\endgroup\$

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