5
\$\begingroup\$

My program does exactly what I want it to do and it works well. However, I feel like it's very clunky.

I'd like my code to be more efficient. By that I mean, I'd like it to accomplish what it already can, but in the best way possible and in the least amount of code. Basically, optimized.

I'd also like it to be easily understandable.

using CefSharp;
using CefSharp.WinForms;
using System.Collections;
using System.Collections.Generic;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Reddit_Newest_Submissions
{
    public partial class Form1 : Form
    {
        private IList newestSubmissionsTitles;
        private IList newestSubmissionsURLs;
        private List<string> filteredNewestSubmissionsTitles = new List<string>();
        private List<string> filteredNewestSubmissionsURLs = new List<string>();

        private readonly ChromiumWebBrowser browser;

        public Form1()
        {
            InitializeComponent();
            browser = new ChromiumWebBrowser("https://www.reddit.com/new/") { Dock = DockStyle.Fill };
            browser.LoadingStateChanged += browser_LoadingStateChanged;
            panel1.Controls.Add(browser);
        }

        // javascript code that will return all of the newest submission's titles
        private const string getNewestSubmissionsTitlesScript = @"var newestSubmissionsTitles = [];

                                                                  for (i = 0; i < document.getElementsByClassName('title may-blank').length; i++) {
                                                                  newestSubmissionsTitles.push(document.getElementsByClassName('title may-blank')[i].innerText);
                                                                  }

                                                                  (function() {
                                                                  return newestSubmissionsTitles;
                                                                  })();";

        //javascript code that will return all of the newest submission's urls
        private const string getNewestSubmissionsURLsScript = @"var newestSubmissionsURLs = [];

                                                                for (i = 0; i < document.getElementsByClassName('title may-blank').length; i++) {
                                                                newestSubmissionsURLs.push(document.getElementsByClassName('title may-blank')[i].href);
                                                                }

                                                                (function() {
                                                                return newestSubmissionsURLs;
                                                                })();";

        //lower down in the code i link each title with the according url based on their index

        private bool gotNewestSubmissions;

        private ListViewItem lvi;

        private void GetNewestSubmissions()
        {
            browser.EvaluateScriptAsync(getNewestSubmissionsTitlesScript).ContinueWith(a =>
                browser.EvaluateScriptAsync(getNewestSubmissionsURLsScript).ContinueWith(b =>
                {
                    var aResponse = a.Result;
                    var bResponse = b.Result;

                    if (aResponse.Success && aResponse.Result != null && bResponse.Success && bResponse.Result != null)
                    {
                        //store the javascript response results into the according lists
                        newestSubmissionsTitles = (IList)aResponse.Result;
                        newestSubmissionsURLs = (IList)bResponse.Result;

                        //check every title from the javascript response to see if it has already been added into listview1 to avoid duplicates
                        foreach (var newestSubmissionsTitle in newestSubmissionsTitles)
                        {
                            Invoke((MethodInvoker)delegate
                           {
                               ListViewItem item = listView1.FindItemWithText(newestSubmissionsTitle.ToString());
                               if (item != null)
                               {
                                   // item exists
                               }
                               else
                               {
                                   // item doesn't exist

                                   // add the title to the filtered titles list
                                   filteredNewestSubmissionsTitles.Add(newestSubmissionsTitle.ToString());
                               }
                           });
                        }

                        //check every url from the javascript response to see if it has already been added into listview1 to avoid duplicates
                        foreach (var newestSubmissionsURL in newestSubmissionsURLs)
                        {
                            Invoke((MethodInvoker)delegate
                           {
                               ListViewItem item = listView1.FindItemWithText(newestSubmissionsURL.ToString());
                               if (item != null)
                               {
                                   // item exists
                               }
                               else
                               {
                                   // item doesn't exist

                                   // add the url to the filtered urls list
                                   filteredNewestSubmissionsURLs.Add(newestSubmissionsURL.ToString());
                               }
                           });
                        }

                        // for loop that will run until all of the filtered submissions have been dealt with
                        for (int i = 0; i < filteredNewestSubmissionsTitles.Count; i++)
                        {
                            Invoke((MethodInvoker)delegate
                           {
                               ListViewItem item1 = listView1.FindItemWithText(filteredNewestSubmissionsTitles[i]);
                               if (item1 != null)
                               {
                                   // item exists
                               }
                               else
                               {
                                   // item doesn't exist

                                   // create a new listviewitem with the index of i that contains a reddit submission title
                                   // this will be added to the 1st column of listview1
                                   lvi = new ListViewItem(filteredNewestSubmissionsTitles[i]);
                               }
                           });

                            Invoke((MethodInvoker)delegate
                           {
                               ListViewItem item2 = listView1.FindItemWithText(filteredNewestSubmissionsURLs[i]);
                               if (item2 != null)
                               {
                                   // item exists
                               }
                               else
                               {
                                   // item doesn't exist

                                   // adds a listviewitem with the index of i that contains a reddit submission url
                                   // this will be added to the 2nd column of listview1
                                   lvi.SubItems.Add(filteredNewestSubmissionsURLs[i]);
                               }
                           });

                            //check to see if the title + url listviewitem has already been added to avoid duplicates
                            Invoke((MethodInvoker)delegate
                           {
                               bool exists = false;
                               foreach (ListViewItem item in listView1.Items)
                               {
                                   if (item == lvi)
                                       exists = true;
                               }

                               if (!exists)
                                   listView1.Items.Add(lvi);
                           });
                        }

                        gotNewestSubmissions = true;
                    }
                }));
        }

        private bool redditLoaded;

        private void browser_LoadingStateChanged(object sender, LoadingStateChangedEventArgs e)
        {
            // if the browser has finished loading
            if (e.IsLoading == false)
            {
                browser.LoadingStateChanged -= browser_LoadingStateChanged;

                // if the loaded url is reddit's newest submissions page
                if (browser.Address == "https://www.reddit.com/new/")
                {
                    redditLoaded = true;
                }

                browser.LoadingStateChanged += browser_LoadingStateChanged;
            }
        }

        // timer that will constantly be grabbing reddit's newest submissions
        private async void timer1_Tick(object sender, System.EventArgs e)
        {
            if (redditLoaded)
            {
                timer1.Stop();

                while (gotNewestSubmissions == false)
                {
                    GetNewestSubmissions();
                    await Task.Delay(5000);
                }

                await Task.Delay(10000);
                gotNewestSubmissions = false;
                redditLoaded = false;
                browser.Load("https://www.reddit.com/new/");

                timer1.Start();
            }
        }
    }
}

Here is what my program looks like after running for about 20 seconds:

enter image description here

I use the beloved CefSharp for my web browser.

\$\endgroup\$
5
\$\begingroup\$
 Invoke((MethodInvoker)delegate
{
    bool exists = false;
    foreach (ListViewItem item in listView1.Items)
    {
        if (item == lvi)
            exists = true;
    }

    if (!exists)
        listView1.Items.Add(lvi);
});

This single snippet of code has many things that can be improved in both readability, performance, and maintainability.

First, use braces around your single-statement blocks. This will make it much easier for you to maintain later, and will ensure bugs are not introduced when more statements are added.

Second, you are overcomplicating things with that loop. First, you loop way more than necessary:

foreach (ListViewItem item in listView1.Items)
{
    if (item == lvi)
        exists = true;
}

You should break as soon as you know you have an item to prevent the loop from spinning through unnecessary cycles:

foreach (ListViewItem item in listView1.Items)
{
    if (item == lvi)
    {
        exists = true;
        break;
    }
}

But wait a second - there is a built-in way to do this in a single statement! This method is in the System.Linq namespace, and can be written like:

bool exists = listView1.Items.Any(item => item == lvi);

This is essentially a built-in method that does exactly what my loop with the early-exit does.

Next, while your indentation is pretty good, your Invoke statement is indented one space farther than it should be.

Finally, your naming could be improved. What kind of data does listView1 contain? It should be named something like redditSubmissions, or something descriptive.


var aResponse = a.Result;
var bResponse = b.Result;

Your naming is bad here too. What are aResponse and a? Also, please be consistent in your use of var, bool, and more. Use var in cases like var exists = false, where the type is easily determinable. Use the explicit type name when it is not easily determinable (or only when the compiler cannot determine the type).


if (item != null)
{
    // item exists
}
else
{
    // item doesn't exist

    // add the title to the filtered titles list
    filteredNewestSubmissionsTitles.Add(newestSubmissionsTitle.ToString());
}

Your if does absolutely nothing except make this difficult to read. Even the compiler knows it does nothing and compiles it away. Just write this like:

if (item == null)
{
    filteredNewestSubmissionsTitles.Add(newestSubmissionsTitle.ToString());
}

Your comments are redundant because it is clear you are adding it to the list if it does not exist.

You did use braces here, which is good. Please be consistent whatever you choose, though, and either always use them or never used them when they are not needed.

\$\endgroup\$

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.