Injecting JavaScript to website for ordering products

I am a newbie at programming and I helped my dad with making this program which orders multiple products to a website from a table of Excel.

It involves 3 processes:

-
Passing orders from Excel to website by injecting JavaScript.

The website provides a function for ordering a product: (in JavaScript)

updateProduct('{reference}','0','/products/update'{quantity},110,'divAlertQteDispoMessage',0,900,'divAlertNbRowsMessage');

-
Comparing the product list between the website and the Excel sheet.

My current solution is to get all reference id found in website and compare it with the Excel's reference id

-
Deleting products which have been ordered before.

Using .cmd according to the reference, we can get the quantity of the product ordered before.

Here is the entire class:

```
using System;
using System.Collections.Generic;
using System.Data;
using System.Data.OleDb;
using System.Drawing;
using System.IO;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using Awesomium.Core;
using Awesomium.Windows.Controls;
using ClosedXML.Excel;
using FirstFloor.ModernUI.Windows.Controls;
using Brushes = System.Windows.Media.Brushes;
using SystemFonts = System.Windows.SystemFonts;

namespace ModernUINavigationApp1.Pages
{

///
/// Interaction logic for Home.xaml
///
///

public partial class Home : UserControl
{

private Microsoft.Win32.OpenFileDialog dlg = new Microsoft.Win32.OpenFileDialog();

public Home()
{
InitializeComponent();
Startup();
MainwebBrowser.ViewType = WebViewType.Offscreen;

}

private async void Startup()
{
await

Alright, let's go through the code top-to-bottom.

using Brushes = System.Windows.Media.Brushes;
using SystemFonts = System.Windows.SystemFonts;

Any particular reason you're using this alias? Most people I see that use aliases (including me) have these as part of an automatic re-formatting of the document.

private Microsoft.Win32.OpenFileDialog dlg = new Microsoft.Win32.OpenFileDialog();

As far as I can tell you only show the dialog once (to get the location) and afterwards you only use the filename. I would keep a local field pointed to the filename and create the dialog inside BrowseButton_Click instead. That way you can also easily check for if(filename == null) instead of doing if (string.IsNullOrEmpty(TextBoxFile.Text)).

private async void Startup()

async void is only acceptable in two situations: events and partial methods. Async void methods are bad (tricky?) because you can't actually await them: they are fire-and-forget. Always make them return Task and perhaps use a library like AsyncEx to create an asynchronous context to work from.

await Task.Delay(2000);

This is a pointless delay. Why start your program and immediately put everything on hold? You're not waiting for anything to load but if you are then you should make that very clear (both to the user and the programmer).

while (!MainwebBrowser.IsDocumentReady)
{
    await Task.Delay(1000);
}

I'm not familiar with the library but isn't there an event you can subscribe to instead? I notice something like LoadingFrameComplete but there could be others too. Either way I would decrease the delay to, say, 100ms for better responsiveness and no difference in performance.

await Task.Delay(1000);

Again a seemingly pointless sleep.

string filename = dlg.FileName;
TextBoxFile.Text = filename;
DataTable dt = LoadXls(dlg.FileName);

I don't think it's likely for race conditions to pop up here but it's still a nice habit to take with you: what would happen if dlg.FileName changed its value between line 2 and 3? You would load the wrong data! I would suggest re-using the filename variable.

General note about naming: keep things descriptive. dlg, dt, _label, etc are all bad names because they don't convey what they are about. Don't abbreviate (unless a few specific known ones like db) and keep things expressive.

public bool _checkcancelled;

Expose variables using properties. That way you adhere to the "encapsulation" principle of Object Oriented programming. By using properties you can always decide later to provide setter/getter implementation without breaking the contract.

Naming conventions!

Look through the list here but keep in mind that private non-static, non-const fields are also accepted to be _lowerCamelCase.

I typically prefer string.IsNullOrWhiteSpace over string.IsNullOrEmpty because the former also takes care of.. well.. whitespace characters.

if (MainwebBrowser.Source != new Uri("http://www.easyrea.net/cart/detail"))
{
    MainwebBrowser.Source = new Uri("http://www.easyrea.net/cart/detail");
}

Nobody likes to copy things twice. It's extra work and prone to errors. Instead, define it once:

var targetUri = new Uri("http://www.easyrea.net/cart/detail");
if (MainwebBrowser.Source != targetUri)
{
    MainwebBrowser.Source = targetUri;
}

You're logging in in a method called DeleteProduct() -- this violates the method's Single Responsibility Principle. Perform the logging in in a separate method, this will also be easier for you to keep track of what happens where. On top of that, you also won't have to repeat yourself each time you want to log in.

Feels a little pointless to inject a JS function and then only call it once. Can't you directly call if (document.getElementById('topNav') != null) { return 'true'; } from ExecuteJavascriptWithResult()? I would use an intermediate variable to store the string for readability purposes.

I've noticed you're loading that datatable many times. Have you considered loading it once, doing the reading from that and only reloading it when you actually modify the data source?

if (cmd != "" & existed != "")

Stick to one way of comparison: string.IsNullOrX would be more conform to the rest of the code.

int intcmd;
bool isintcmd = int.TryParse(Regex.Match(cmd, @"\d+").Value, out intcmd);

int intexisted;
bool isintexisted = int.TryParse(existed, out intexisted);
if (isintcmd & isintexisted)

This could instead be written as

int intcmd = int.Parse(Regex.Match(cmd, @"\d+").Value);
int intexisted;
if (int.TryParse(existed, out intexisted))

You don't really need the boolean results so you might as well use them in the if directly. Since your regex matches only numbers, you will only receive numbers and your parse will succeed either way (aka: no need for the TryParse).

_label3.Content = "";

I prefer string.Empty because it clearly conveys y

StackExchange Code Review Q#99026, answer score: 8