Script that parses the Chase.com page for my recent payments

Question

I wrote this code earlier to parse Chase.com's online transactions page. It's written in WinForms.

stepBtn is a button that starts this.
wb is a WebBrowser that is already navigated to the page.

Can you give me advice? I'm primarily looking for optimizations, but any other advice would be great.

private void stepBtn_Click(object sender, EventArgs e)
{
    List<string> date = new List<string>();
    List<string> desc = new List<string>();
    List<double> amt = new List<double>();
    //Parse for date
    var links = wb.Document.GetElementsByTagName("span");
    foreach (HtmlElement link in links)
    {
        if (link.InnerText != null)
        {
            string inner = link.InnerText.Trim();
            if (inner == "Pending" || Regex.IsMatch(inner, @"^(0[1-9]|1[012])[- \/.](0[1-9]|[12][0-9]|3[01])[- \/.](19|20)\d\d$"))
            {
                date.Add(inner);
            }
        }
    }

//Parse for description
links = wb.Document.GetElementsByTagName("td");
foreach (HtmlElement link in links)
{
    if (link.GetAttribute("classname") == "cellStyle")
    {
        desc.Add(link.InnerText);
    }
}

//Parse for amount
//Assuming I never make a $1000+ purchase, every number < 1000 will be shown. That way, the balance is not included.
links = wb.Document.GetElementsByTagName("td");
int times = 0; //There are two $xxxx.xx before what I want to parse, the present and available balance. 
foreach (HtmlElement link in links)
{
    if (times > 2 && link.InnerText != null && link.InnerText.Trim().Length >= 5)
    {
        string inner = link.InnerText.Trim();
        if (inner.Substring(0, 1) == "$" && Convert.ToDouble(inner.Substring(1)) < 1000)
        {
            amt.Add(Convert.ToDouble(inner.Substring(1)));
        }
    }
    times++;
}

//Check to make sure all lists have the same length
if (date.Count != amt.Count || amt.Count != desc.Count)
{
    MessageBox.Show("The three arrays do not have the same length.\n\nDate: " + date.Count + "\nAmt: " + amt.Count + "\nDesc: " + desc.Count, Text, MessageBoxButtons.OK, MessageBoxIcon.Error);
    return;
}

//Output the arrays to the first checklist
for (int i = 0; i < date.Count; i++)
{
    firstCheck.Items.Add(date[i] + ": " + desc[i] + " (" + amt[i] + ")");
}
}

Heslacher · Accepted Answer · 2015-06-10 07:01:49Z

Single responibility principle

You method violates the SRP because it does to many things.

it parses HtmlElements for 3 different types
it validates the resulting List<T>
updates the UI

If you need comments like

//Parse for date
//Parse for description
//...

then it is a clear sign that these parts should be extracted to separate methods.

A much cleaner way would be

private const string dateRegexPattern = @"^(0[1-9]|1[012])[- \/.](0[1-9]|[12][0-9]|3[01])[- \/.](19|20)\d\d$"
private List<string> ParseForPurchaseDates(HtmlDocument document)
{
    var purchaseDates = new List<string>();

    var links = document.GetElementsByTagName("span");

    foreach (HtmlElement link in links)
    {
        if (link.InnerText == null) { continue; }

        string inner = link.InnerText.Trim();
        if (inner == "Pending" || Regex.IsMatch(inner,dateRegexPattern ))
        {
            purchaseDates.Add(inner);
        }

    }
    return purchaseDates;
}

if you do this for the other parsings too, your click eventhandler could look like

private void stepBtn_Click(object sender, EventArgs e)
{
    List<string> date = ParseForPurchaseDates(wb.Document);
    List<string> desc = ParseForPurchaseDescriptions(wb.Document);
    List<double> amt = ParseForPurchaseAmounts(wb.Document);

    if (ListCountIsValid(date, desc, amt))
    {
        // update the UI
        return;
    }

    MessageBox.Show("The three arrays do not have the same length.\n\nDate: " + date.Count + "\nAmt: " + amt.Count + "\nDesc: " + desc.Count, Text, MessageBoxButtons.OK, MessageBoxIcon.Error);
}

private bool ListCountIsValid(List<string> purchaseDates, List<string> purchaseDescriptions, List<double> purchaseAmounts)
{
    return purchaseDates.Count == purchaseAmounts.Count  
        && purchaseDescriptions.Count == purchaseAmounts.Count;
}

For the parsing of the amount I would like to suggest to add a second parameter which should be optional to replace the hardcoded value 1000.

private List<double> ParseForPurchaseAmounts(HtmlDocument document, double maxAmount = 1000d)
{
    //....
}

Naming

Naming is a very important part of programming. Using abbreviations instead of the complete word will reduce readability and makes it hard to maintain. So give Bob the maintainer a chance to do his/her job easy and fast by using proper names.

amt should be e.g amounts or purchaseAmounts ...

BCdotWEB · Accepted Answer · 2015-06-10 11:51:16Z

There are other naming issues besides the ones Heslacher mentions in his review. For instance: the return value of wb.Document.GetElementsByTagName("span"); is stored in a variable named links -- shouldn't that be spans?

Or if you want to re-use this variable to store the return value of wb.Document.GetElementsByTagName("td");, why not name it elements?

But I'm more worried by the logic of the code: process the three types of data separately into List<string>s, and then combine them at the end.

I'm not familiar with the code of the page you're parsing, but I'd be surprised if each of the three elements you need isn't contained in one block (a div for instance, or a tr). Then you could focus on extracting the relevant data for one entry from said block, thus ensuring it does belong together.

Because right now you have no clue why you'd get "The three arrays do not have the same length." and you would need to investigate which of the entries on the page doesn't follow the expected format. Whereas if you know that the culprit is one particular block, you wouldn't waste time on finding the source of your problem: you could immediately focus on the problematic block.

You wouldn't even need to change that much: identify the tag that "defines" such a block and then use HtmlElement.GetElementsByTagName to retrieve its parts instead of HtmlDocument.GetElementsByTagName. Store the resulting data in a custom class:

public class AccountEntry
{
   public string Date { get;set; }
   public string Amount { get;set; }
   public string Description { get;set; }
}

I would even consider using the correct types for Date (DateTime) and Amount (decimal), though that seems to be a bit OTT for your purpose.

Stack Exchange Network

Script that parses the Chase.com page for my recent payments

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Script that parses the Chase.com page for my recent payments

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions