Load & view movies on a hard drive in Netflix-like UI

I am a self-taught programmer and would like anyone to point out anything that is not the standard or professional way to do things. Any help/advice is greatly appreciated.

I would love to post a lot of my code or perhaps make a youtube video explaining certain portions for feedback. Let me know in the comments what you think.

The specific code I would like feedback for is how I take the movies I have found and add them to the listView. This takes the most time and is roughly 1 movie every second.

To start you hit Load.

private void button13_Click(object sender, EventArgs e)
    {
        // if (bgwPopulateMyMovies.IsBusy == false)
        // {
        //     bgwPopulateMyMovies.RunWorkerAsync();
        // }
        Thread movieThread = new Thread(new ThreadStart(populateVideosGrid));
        movieThread.Start();
        labelTotalMyVideos.Text = "My Videos: " + dgwMyVideos.Rows.Count.ToString();

    }

I originally had the code in a backgroundWorker, but it ran substantially slower.

This triggers my populateVideosGrid() method. Which will get the movies from a dataGrid on my other tab that is populated with movies.

private void populateVideosGrid()
    {
        for (int i = 0; i = dgwMyVideos.Rows.Count)
                return;

            string moviePath = "";
            string Year = "";
            string Season = "";
            string Episode = "";
            try
            {
                moviePath = dgwMyVideos.Rows[i].Cells[1].Value.ToString();
                Year = dgwMyVideos.Rows[i].Cells[2].Value.ToString();
                Season = dgwMyVideos.Rows[i].Cells[4].Value.ToString();
                Episode = dgwMyVideos.Rows[i].Cells[5].Value.ToString();
                if (moviePath != "")
                    addToMyVideos(moviePath, Year, Season, Episode);
            }
            catch (Exception)
            {
            }
        }
    }

Where I loop through the dataGrid and call addToMyVideos method.

```
private vo

First of all: why a Windows Forms application? IMHO it's outdated technology, and you should look at WPF and beyond.

The first bit of code you post already shows a bad practice: button13_Click is obviously linked to a button called button13, and that name is utterly meaningless. You say that this is what is triggered when you hit Load -- so why then not call it a LoadButton?

Please read up on the capitalization conventions: methods -- populateVideosGrid() for instance -- should be PascalCase.

But this method name also exposes how deeply your application logic is tied to your UI, and that's a bad thing. If you had used WPF, you could have implemented MVVM but this kind of separation is much harder to do in WF.

Even though populateVideosGrid() is only twenty-odd lines long, there's much more wrong with it. You use try... catch but ignore the exception: that's bad. But I wonder why there's even the possibility of an exception happening?

Why do you use a for loop instead of a foreach? You should also move the logic inside the try to a separate method which accepts a Row as the parameter.

if (moviePath != "") is odd. Why not use string.IsNullOrEmpty()? Also, why even bother assigning Year, Season and Episode if moviePath doesn't contain a value?

Year, Season and Episode should be camelCase.

Why do you need to specify the whole namespace in TMDbLib.Objects.Movies.Movie aMovie? And why is your variable named aMovie -- why not just name it movie?

justMovieName is a bad method name, since it lacks a verb or verb phrase.

Parameters should be camelcase: addToMyVideos(string Name, string Year, string Season, string Episode).

You say:


This is where I believe the majority of the bottle neck is as I have
to use the TMDbLib API to pull movie information. Every. Single. Time.

Aren't there batch methods available? Can't you cache data for movies in a local db or an XML file so you only need to look up movies that aren't stored in that db or file?

Also: are you seriously retrieving "image-unavailable.jpg" every time? Why? Do you expect it to change form one second to the other? Why not get it once and store it on your HD and retrieve it from there?

The main problem I see is that you haven't separated your concerns. Read up on n-tier architecture and realize that you shouldn't tie your UI to your other logic.

Consider this approach:

• All your UI needs is a list of "movies"

• By "movies" I mean classes with a number of properties:

• their location on disc,

• the local path to an image,

• the title etc.

• That information is stored locally,

• and if it isn't you need to retrieve it from TMDb and then store it locally.

Work with smaller blocks of logic, each designed for a specific function. Start from there, and then try to improve on your logic. For instance:

• perhaps the exception that occurs in AddOneEntryToMyVideos might be solved later on,

• so you'll need to re-query TMDb at a later time -- but not each time the screen refreshes,

• which means you need to store a date at which you last queried TMDb

• and only query it again once a certain amount of time has passed.

Also look into async operations and parallelism, in order to execute multiple queries to TMDb at the same time.

StackExchange Code Review Q#118021, answer score: 5