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.