Here are my suggestions:
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.
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
Don't put two executable statements in the same line.
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,
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.
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):
Application.ScreenUpdating = False
Dim url as String
Dim proxyServer As Variant
Dim bypassList as Variant
url = "https://yts.ag/browse-movies"
proxyServer = "126.96.36.199:80"
bypassList = "188.8.131.52:8118"
Dim html As New HTMLDocument
html.body.innerHTML = GetHttpResponseTextByProxy(url, proxyServer, bypassList)
Call SetMovieData(html, ActiveSheet)
Application.ScreenUpdating = True
Private Function GetHttpResponseTextByProxy( _
ByRef url as String, _
ByRef proxyServer as Variant, _
ByRef bypassList as Variant) As String
Dim http As New ServerXMLHTTP60
.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
GetHttpResponseTextByProxy = http.responseText
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