Preparing a YouTube downloader based on Windows form

Question

The program this function was written for is a Windows forms program.

This function downloads videos from the internet one after the other. It runs in a separate thread (so the user can drag the form around or whatever), but otherwise the form is disabled, so no more instances can be created without doing so explicitly in the code.

It does not, in fact, download the video. Instead, it sets up the function that will download the video, including all restrictions and such that relate to the user's options before clicking "download".

However, I feel like the function is bloated and clunky, among other concerns. Can someone help me increase its efficiency?

public void SetupDownloadingProcess(int retryCount, Collection<Tuple<string, int, VideoType>> urlList)
 {
 int previouslySelectedIndex = MainForm.SelectedQueueIndex;
 ObservableCollection<Tuple<string, int, VideoType>> finishedUrlList = new ObservableCollection<Tuple<string, int, VideoType>>();
 if (MainForm.UrlListNumberItems > 0)
 {
 MainForm.StartDownloadingSession(true);
 int position = 0;
 for (int count = 0, GlobalVariablesurlListCount = urlList.Count; count < GlobalVariablesurlListCount; count++)
 {
 if(MainForm.CurrentlyDownloading)
 {
 try
 {
 Tuple<string, int, VideoType> url = urlList[count];
 MainForm.SelectedQueueIndex = position;
 position++;
 Tuple<string, int, VideoType> tempTuple = DownloadVideos(url, position);
 if (tempTuple != null)
 {
 finishedUrlList.Add(tempTuple);
 }
 Storage.WriteUrlsToFile(finishedUrlList, true);
 }
 catch(Exception ex)
 {
 var exceptionMessage = ex.Message;
 if(retryCount <= 3)
 {
 MainForm.StatusBar = string.Format(CultureInfo.InstalledUICulture, "URL {0}: {1}. Retrying.... ({2})", count + 1, Trunicate(exceptionMessage, 50), retryCount < 3 ? (retryCount + 1).ToString(CultureInfo.CurrentCulture) : "Final Try");
 System.Threading.Thread.Sleep(850);
 SetupDownloadingProcess((retryCount + 1), urlList);
 }
 else
 {
 finishedUrlList.Clear();
 MainForm.StatusBar = Trunicate(exceptionMessage, 100);
 }
 }
 }
 }
 }
 MainForm.CurrentlyDownloading = false;
 MainForm.StartDownloadingSession(false);
 MainForm.StartDownButtonEnabled = true;
 GlobalVariables.urlList = GetUnfinishedDownloads(finishedUrlList);
 Storage.WriteUrlsToFile(GlobalVariables.urlList, false);
 MainForm.RefreshQueue(previouslySelectedIndex, true);
 }

I prefer not to post all separate references as that would take forever and eat up a lot of room and be boring, but for the truly curious, my entire project can be found here.

I'm still relatively new to C# and coding in general, so please be patient with me and as thorough as possible.

• user3375027: Have I edited it correctly? I believe I have but I'd like to be sure.
• jacwah: The title should be a description of what your code does. You could go with something like "Video download procedure for Windows Forms".
• user3375027: Okay. If there are any more required revisions please let me know.
• jacwah: Looks great now!

Answer 1

Perhaps it's a copy-paste error, but please reduce the blank lines in your code. Simply removing most of them reduces this example with one third.

---

Is there a reason why you use WinForms? Quite frankly I feel the technology is outdated; WPF offers you far better ways to do the same thing. For instance, you can use MVVM.

---

Why use Tuple<string, int, VideoType> when it would be far clearer to create a custom class? The next person to look at this code will first need to figure out what the string and the int represent, whereas if this had been a custom class they'd know the first was the URL and the second... well, I don't know what, but you get my drift.

---

Trunicate? Do you mean Truncate?

---

GlobalVariablesurlListCount is a horrible name. A property should never start with "GlobalVariables". Later on there's a GlobalVariables.urlList, so now I'm wondering if GlobalVariablesurlListCount is a typo.

Even if that's the case, having a class called GlobalVariables to pass data around indicates a bad design, and its properties don't seem to follow the capitalization guidelines: urlList should be PascalCase.

Also, urlList shouldn't be called "List", even if it is a List<T>. Just call it Urls.

---

I realize naming is often hard, but please avoid the temptation to name things "temp", e.g. tempTuple.

Also, why is the method named DownloadVideos when I assume it only downloads a single video?

And why would DownloadVideos return a tuple? Is this the same tuple it receives as a parameter?

(Note that I did not look at your full code at the link you provided.)

---

The string.Format that assigns a status to MainForm.StatusBar is 200+ characters long and contains a conditional operator. IMHO it is far to messy and far too hard to understand. Is there even a good reason to display "Final Try"? Why not simply have "(1/3)", which would also inform the user how many retries are being attempted?

---

Your method is called SetupDownloadingProcess but it does far more than that, doesn't it? It interacts with the UI, it logs urls that were downloaded, it even retries the download attempt. There's a lot here that looks like functionality that should be handled differently; it almost looks like you've stumbled upon mimicking a queue or a background worker.

• user3375027: There's a whole lot of work ahead of me. :)