patterncsharpMinor
Newest Reddit submissions grabber
Viewed 0 times
newestsubmissionsredditgrabber
Problem
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 filteredNewestSubmissionsTitles = new List();
private List filteredNewestSubmissionsURLs = new List();
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
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.R
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 filteredNewestSubmissionsTitles = new List();
private List filteredNewestSubmissionsURLs = new List();
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
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.R
Solution
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.
Code Snippets
Invoke((MethodInvoker)delegate
{
bool exists = false;
foreach (ListViewItem item in listView1.Items)
{
if (item == lvi)
exists = true;
}
if (!exists)
listView1.Items.Add(lvi);
});foreach (ListViewItem item in listView1.Items)
{
if (item == lvi)
exists = true;
}foreach (ListViewItem item in listView1.Items)
{
if (item == lvi)
{
exists = true;
break;
}
}bool exists = listView1.Items.Any(item => item == lvi);var aResponse = a.Result;
var bResponse = b.Result;Context
StackExchange Code Review Q#119713, answer score: 5
Revisions (0)
No revisions yet.