2
\$\begingroup\$

I've written a script which is scraping a webpage encrypted within javascript. The scraper is extracting name, price and image link of certain products from that page. I used selenium in combination with vba to accomplish that. Hope I did it nicely.

Sub Redmart_scraping()
Dim driver As New WebDriver
Dim posts As Object, post As Object

Set driver = New WebDriver
driver.Start "Phantomjs", "https://redmart.com"
driver.get "/bakery"
Set posts = driver.FindElementsByXPath("//li[@class='productPreview  ']")
On Error Resume Next
For Each post In posts
    i = i + 1
    Cells(i, 1) = post.FindElementByXPath(".//h4/a").Text
    Cells(i, 2) = post.FindElementByXPath(".//span[@class='ProductPrice__price___3BmxE']").Text
    Cells(i, 3) = post.FindElementByXPath(".//img[contains(@class,'showImage')]").Attribute("src")
Next post
End Sub
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg May 27 '17 at 10:06
1
\$\begingroup\$

I would improve on handling class attributes. They are multi-valued attributes per definition and your way of checking the class values in the XPath expressions can potentially cause problems.

For instance, @class='productPreview ' check would fail if there will be one space less at the end of the value. Or, you would also match an element with class="showImageDescription" with the contains(@class,'showImage') check.

One way of dealing with that is to use this workaround which involves using concat().

I would though simply switch to CSS selectors which are generally faster, more concise and handle class attributes naturally:

Set posts = driver.FindElementsByCss("li.productPreview")
On Error Resume Next
For Each post In posts
    i = i + 1
    Cells(i, 1) = post.FindElementByCss("h4 > a").Text
    Cells(i, 2) = post.FindElementByCss("span[class^=ProductPrice__price]").Text
    Cells(i, 3) = post.FindElementByCss("img.showImage").Attribute("src")
Next post

Note the span[class^=ProductPrice__price] selector - I don't think the ___3BmxE part of the class matters much and provides any relevant information - plus, it sounds like a good subject for a change. That's why I've switched to a partial match with "starts with" (^=).

\$\endgroup\$
  • \$\begingroup\$ Thanks sir alecxe, for your response. It is always a pleasure to find you in the loop. I have never worked with css selector used within selenium vba. For some reason the code breaks when it hits the first line of your given solution ("set posts=---") showing "object doesn't support this method". \$\endgroup\$ – SIM May 27 '17 at 5:05
  • \$\begingroup\$ @SMth80 thank you for the kind words and a good question. Unfortunately, I am not familiar with selenium on VBA..could it be a case of the first letter - findElementsByCssSelector vs FindElementsByCssSelector?.. \$\endgroup\$ – alecxe May 27 '17 at 5:08
  • \$\begingroup\$ Still no luck, sir. \$\endgroup\$ – SIM May 27 '17 at 5:16
  • \$\begingroup\$ @SMth80 ah, looks like the Selector part at the end was not needed, updated the answer - check it out. \$\endgroup\$ – alecxe May 27 '17 at 13:15
  • \$\begingroup\$ I was not around my pc. Checked it with your updated code. Works perfect. Selector was the culprit then. Thanks again. \$\endgroup\$ – SIM May 27 '17 at 16:37
1
\$\begingroup\$

Haven't really worked with selenium, but I believe you instantiate the WebDriver object twice:

Dim driver As New WebDriver 
'...
Set driver = New WebDriver

On a personal note, I avoid the auto-instancing approach. The object isn't really created, but creation occurs when it is first encountered in code. In addition, you cannot properly test for Nothing.

Dim driver As New WebDriver 
If driver Is Nothing Then
'Simple testing for Nothing will create the object thus will never test 
'properly for the Nothing state.

Instead, I prefer to separate the declaration from instantiation:

Dim driver As WebDriver
Set driver = New WebDriver

Edit:

Regarding error handling, you can check the post state before attempting to extract its data.

Something like this:

'...
On Error GoTo ErrProc

For Each post In posts
    If Not post Is Nothing Then
        i = i + 1
        Cells(i, 1) = post.FindElementByXPath(".//h4/a").Text
        Cells(i, 2) = post.FindElementByXPath(".//span[@class='ProductPrice__price___3BmxE']").Text
        Cells(i, 3) = post.FindElementByXPath(".//img[contains(@class,'showImage')]").Attribute("src")
    End If
Next post


Leave:
    'release object references
    Set posts = Nothing
    Set driver = Nothing
    On Error GoTo 0
    Exit Sub

ErrProc:
    MsgBox Err.Description, vbCritical
    Resume Leave
End Sub
\$\endgroup\$
  • \$\begingroup\$ Thanks Kostas K. for your response. Declaration and instantiation are made separately upon your suggestion. Btw, is it pssible to avoid "on error resume next" from my code. It looks weird. \$\endgroup\$ – SIM May 27 '17 at 9:53
  • \$\begingroup\$ @SMth80 Updated answer regarding error handling. Hope it helps. \$\endgroup\$ – Kostas K. May 27 '17 at 10:27

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.