2
\$\begingroup\$

I've written a script in vba to parse movie names and year from a torrent site. The script is doing just awesome. Although the scraper is leaving no room for complaint, I'm still dubious about how the way I've set the proxy is accurate. Moreover, I've set two proxies in my scraper. Btw, I went through few codes where the proxy has been set using .setProxy 2,"xxxxxxx" whereas I used .setProxy 1 in my below code because the earlier one was throwing errors. Once again, the code is working flawlessly. Thanks in advance.

Here is what I've written:

Sub Torrent_Scraper()
    Dim http As New ServerXMLHTTP60, html As New HTMLDocument
    Dim post As HTMLHtmlElement

    With http
        .Open "GET", "https://yts.ag/browse-movies", False
        .setRequestHeader "Content-Type", "text/html; charset=utf-8"
        .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36"
        .setProxy 1, "61.233.25.166:80", "46.101.27.218:8118"
        .send
        html.body.innerHTML = .responseText
    End With

    For Each post In html.getElementsByClassName("browse-movie-bottom")
        With post.getElementsByClassName("browse-movie-title")
            x = x + 1: Cells(x, 1) = .item(0).innerText
        End With
        With post.getElementsByClassName("browse-movie-year")
            If .Length Then Cells(x, 2) = .item(0).innerText
        End With
    Next post
End Sub
\$\endgroup\$
2
\$\begingroup\$

Here are my suggestions:

Use Application.ScreenUpdating = False to speed up the execution of the code. See here for more details.

Declare elements where you need them. While your code is short (which is great, btw), it's a good habit to get into.

Use Option Explicit and declare that x variable appropriately. x right now is of type Variant but you're only using it like a Long, so you should declare it as Long. Also, change x to a more meaningful name like row.

Don't put two executable statements in the same line.

Having x = x + 1: Cells... is clever, but it may confuse other people later on (or even yourself if you don't touch this code for months).

Set a reference to the active sheet if that's the sheet you intend to use. When you use Cells(...) you are referring to the active sheet. However, while you are looping through your items in the For loop, if you were to click on another sheet, then your data will get messed up when you could avoid this issue pretty easily. In the code below, Cells(...) becomes activeWs.Cells(...).

Personally, I would only use With ... End With if I had many statements I needed it for (like the first use you have of it). In the second and third uses of With, I would just remove it. I would also use If ... Then ... End If so the If statement isn't so long.

I can tell that you wanted to make the code as short as possible, but sacrificing a few lines for clarity will be more beneficial in the long run, in my opinion.

Lastly, about If post.getElementsByClassName("browse-movie-year").Length Then, I would explicitly state what you really mean, which is that if the length if greater than 0. While this statement returns an array and I get that you're checking for an array length and it will always be >=0, it's best to be explicit for clarity.

EDIT: Thinking about this further, this code could be considered to do too much. If you were to follow the Single Responsibility Principle, then we could split out the work to other, private functions.

Here is my version of the refactored code (untested):

Option Explicit

Sub Torrent_Scraper()
    Application.ScreenUpdating = False

    Dim url as String
    Dim proxyServer As Variant
    Dim bypassList as Variant

    url = "https://yts.ag/browse-movies"
    proxyServer = "61.233.25.166:80"
    bypassList = "46.101.27.218:8118"

    Dim html As New HTMLDocument
    html.body.innerHTML = GetHttpResponseTextByProxy(url, proxyServer, bypassList)

    Call SetMovieData(html, ActiveSheet)

    Application.ScreenUpdating = True
End Sub

Private Function GetHttpResponseTextByProxy( _
    ByRef url as String, _
    ByRef proxyServer as Variant, _
    ByRef bypassList as Variant) As String

    Dim http As New ServerXMLHTTP60

    With http
        .Open "GET", url, False
        .setRequestHeader "Content-Type", "text/html; charset=utf-8"
        .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36"
        .setProxy 1, proxyServer, bypassList
        .send
    End With

    GetHttpResponseTextByProxy = http.responseText
End Function

Private Sub SetMovieData(ByRef html as HTMLDocument, ByRef ws As Worksheet)
    Dim post As HTMLHtmlElement
    Dim row As Long

    For Each post In html.getElementsByClassName("browse-movie-bottom")
        row = row + 1
        ws.Cells(row, 1) = post.getElementsByClassName("browse-movie-title").item(0).innerText

        If post.getElementsByClassName("browse-movie-year").Length > 0 Then
            ws.Cells(row, 2) = post.getElementsByClassName("browse-movie-year").item(0).innerText
        End If
    Next post
End Sub
\$\endgroup\$
  • \$\begingroup\$ Thanks joseph4tw, for such an explicitly detailed review. Your explanation brings the clarity of few things that I really missed so long. However, do you think the usage of proxy is right in my script. Although it is working, I came across few usage where I found it being used as setProxy 2, whereas I used seProxy 1,. Thanks again. \$\endgroup\$ – SIM Sep 18 '17 at 16:06
  • \$\begingroup\$ Hi @Shahin, unfortunately I'm not an expert in this area, however, this documentation might help you: msdn.microsoft.com/en-us/library/ms760236(v=vs.85).aspx I wish I could be of further help here. \$\endgroup\$ – Joseph Sep 18 '17 at 18:20
  • \$\begingroup\$ You have already helped me a lot. Thanks. \$\endgroup\$ – SIM Sep 18 '17 at 18:21

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.