6
\$\begingroup\$

I've written a script in VBA in combination with selenium which is able to scrape all the 1000 links from a lazy-loading webpage which displays it's content 20 at a time and doesn't display the full content until it reaches the bottom of that page. However, my script can make this webpage scroll to the end. After collecting all the links from main page, it then follows each individual link to scrape the Name of CEO and Web address of that organization. It is working perfectly now. I tried to make the whole thing flawless. Here is the full code:

Sub Fortune_parser()

Dim driver As New WebDriver
Dim posts As Object, post As Object, items As Object, item As Object

driver.Start "chrome", "http://fortune.com/fortune500"
driver.get "/list/"

driver.Wait 500

For y = 0 To 200

    driver.ExecuteScript ("window.scrollTo(0, document.body.scrollHeight);")
    driver.Wait 500

Next y

Set posts = driver.FindElementsByXPath("//li[contains(concat(' ', @class, ' '), ' small-12 ')]")

For Each post In posts

    i = i + 1
    item_links = post.FindElementByXPath(".//a").Attribute("href")

    driver.Start "chrome", ""
    driver.get item_links

    driver.Wait 500

    Set items = driver.FindElementsByXPath("//div[contains(@class,'company-info-card-table')]")

    For Each item In items
        x = x + 1
        Cells(x, 1) = item.FindElementByXPath(".//div[contains(@class,'small-7')]/p[@class='remove-bottom-margin']").Text
        Cells(x, 2) = item.FindElementByXPath(".//div[contains(@class,'small-9')]/a").Attribute("href")
        driver.Quit
    Next item
Next post

End Sub
\$\endgroup\$
5
\$\begingroup\$
Sub Fortune_parser()

The procedure is implicitly Public; I'd make it explicitly so. It's also using a rather uncommon casing style I don't even have a name for. In every single VBA type library out there[citation needed], public members are PascalCase.

Note that classes that expose public members that have underscores in their names, cannot be used with the Implements keyword; the underscore has a special meaning in VBA, and thus its use should be avoided in member names.

Dim driver As New WebDriver

If you had Rubberduck installed, this would pop an inspection result about driver being auto-instantiated. The way VBA treats those is rather unsettling (and frankly annoying) - essentially, you can't destroy an object that was declared with As New. Best split declaration and assignment into two distinct instructions:

Dim driver As WebDriver
Set driver = New WebDriver

I see you've avoided the classic beginner trap:

Dim posts As Object, post As Object, items As Object, item As Object

Well done! However posts is declared there, and then assigned 8 instructions later. post is used further down, and items+item are even further. I wouldn't declare all variables at the top of the procedure; by doing that it's easy to declare a variable and then forget about it, and then you're stuck with dangling declarations for things you're not using anywhere.

Consider:

Dim driver As WebDriver
Set driver = New WebDriver
driver.Start "chrome", "http://fortune.com/fortune500"
driver.Get "/list/"
driver.Wait 500

'...

Dim posts As Object
Set posts = driver.FindElementsByXPath("//li[contains(concat(' ', @class, ' '), ' small-12 ')]")

Dim post As Object
For Each post In posts
    '...
Next

That last loop is interesting. Where are i, item_links and x declared? Are you not using Option Explicit?


I think I would make a With block hold the reference to the driver object; that way I'm sure it's destroyed proper.

So scratch the part above about splitting declaration and assignment; don't even declare it, and wrap the whole procedure body with this:

With New WebDriver
    .Start "chrome", "http://fortune.com/fortune500"
    '...
    .Quit
End With

You're not consistent about parentheses; these ones are redundant:

driver.ExecuteScript ("window.scrollTo(0, document.body.scrollHeight);")

Notice the whitespace between the method name and the opening parens; these parentheses are telling VBA to take that string literal and pass it by value to the ExecuteScript method.

Should be:

driver.ExecuteScript "window.scrollTo(0, document.body.scrollHeight);"

That way this method call looks like every other method call you have, and doesn't force any argument to be passed ByVal.


There's some implicit code here:

Cells(x, 1) = item.FindElementByXPath(".//div[contains(@class,'small-7')]/p[@class='remove-bottom-margin']").Text
Cells(x, 2) = item.FindElementByXPath(".//div[contains(@class,'small-9')]/a").Attribute("href")

Unqualified Cells calls like these, implicitly refer to the ActiveSheet, which may or may not be set to a valid reference, and may or may not be the sheet you expect. You're also implicitly assigning to the default property of that Range object (the reference returned by Cells).

Explicit code would be:

ActiveSheet.Cells(x, 1).Value = item.FindElementByXPath(".//div[contains(@class,'small-7')]/p[@class='remove-bottom-margin']").Text
ActiveSheet.Cells(x, 2).Value = item.FindElementByXPath(".//div[contains(@class,'small-9')]/a").Attribute("href")

Your code will work fine, until the scraped website changes their CSS classes. I would extract the CSS class names from the string literals and into a local constant, e.g.:

Const CSS_CLASS_HREF As String = "small-9"

And then concatenate the constant into the xpath query:

item.FindElementByXPath(".//div[contains(@class,'" & CSS_CLASS_HREF & "')]/a").Attribute("href")

That way if/when the CSS changes, you don't need to dig up any specific instructions, you just update your CSS class names constants to mirror the website's changes.


Error handling is missing. What happens when the site is down? When FindElementByXPath returns Nothing? A flawless procedure would handle these sticky situations gracefully.

\$\endgroup\$
  • \$\begingroup\$ Thanks a trillion sir Mat's Mug, for your nicely descriptive review. You are right that I don't use Option Explicit. One important thing is that I thought it would be difficult for me to get the certification of the "for y loop" validity in my script because i had no other alternatives in my mind to forcefully execute the javascript function to scroll the webpage to the end. Thanks again sir for everything. \$\endgroup\$ – SIM Jul 5 '17 at 17:11

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.