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