Good job on your question.

It's good practice to indentall of your code that wayLabels will stick out as obvious.

Dim sUrl, sContent, intMatches

When you don't define your variable, VBA will declare it as aVariant, which areobjects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

Dim a            As IntegerDim b            As IntegerDim c            As IntegerDim d            As IntegerDim i            As IntegerDim boolCtrl     As BooleanDim period       As VariantDim sCrcy        As VariantDim MsgErr       As String

Variable names - give your variables meaningful names.

Integers -integers are obsolete. According tomsdn VBAsilently converts all integers tolong.

boolCtrl - no need for bool

Hungarian naming?Standard VBA naming conventions havecamelCase for local variables andPascalCase for other variables and names.

With Selection    .HorizontalAlignment = xlCenter    .NumberFormat = "@"End With

Be sure to avoid things like.Select - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this -https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros .

'This retrieves values of currency (until end with)

Comments -"code tell you how, comments tell you why". The code should speak for itself, if it needs a comment, it might need to be made more clear. If not, the comment should describewhy you're doing something rather thanhow you're doing it. Here are afew reasons to avoid comments all together.

Function GetRateYear(sFromCrcy, sToCrcy, sYear, a, b)

If possible, you should pass argumentByVal rather thanByRef. ByRef is default.

Your functions should bePrivate instead ofPublic. Public is default.

Functions should be used when something isreturned and subs should be used when somethinghappens.

Private Function GetRateYear(ByVal fromCurrency as String, ByVal toCurrency as String, ByVal year as String, ByVal a as Long, ByVal b as Long) As ??

• \$\begingroup\$WoooooooW thank you so much!! You gave me alot of documentation for me to learn! Thanks!\$\endgroup\$

  svacx

  – svacx

  2016-05-12 16:08:20 +00:00

  CommentedMay 12, 2016 at 16:08
• \$\begingroup\$@svacxpython that's my thing, finding documentation. Once you fix all the stuff mentioned by Mat and I, I hope you post it again so we can look at a lot of the stuff mentioned in the other answer (extracting methods, using an array, etc). I actually took all the low-hanging fruit here and Mat addressed a lot of the meat.\$\endgroup\$

  Raystafarian

  – Raystafarian

  2016-05-12 16:21:23 +00:00

  CommentedMay 12, 2016 at 16:21
• \$\begingroup\$I will modify my code to make it better! But I want it to be perfect! I hope you guys don't mind taking a look at it!\$\endgroup\$

  svacx

  – svacx

  2016-05-12 16:32:05 +00:00

  CommentedMay 12, 2016 at 16:32
• \$\begingroup\$could you please review the code? I've posted it as a answer, as I'm not sure what's the best way: post as an answer or edit the question\$\endgroup\$

  svacx

  – svacx

  2016-05-17 08:05:26 +00:00

  CommentedMay 17, 2016 at 8:05
• 1
  \$\begingroup\$Neither, if you want it reviewed again, go ahead and post another question - it's not really an answer\$\endgroup\$

  Raystafarian

  – Raystafarian

  2016-05-17 08:27:13 +00:00

  CommentedMay 17, 2016 at 8:27

Nitpick, butSub fetchCurrencyPast() should bePublic Sub FetchCurrencyPast(), i.e. using PascalCasing consistent with the rest of module members, and explicitlyPublic for clarity; VBA does so many things behind our backs, it's good to always be explicit where possible!

The first executable statement in that procedure makes an important assumption:

Columns("A:F").Select

TheColumns call is unqualified, which means it operates on whichever worksheet is active when the code runs. Worse,every single worksheet-accessing statement in the procedure is also implicitly referring to the active sheet, which means if the user manages to activate another sheet while the code is running, the macro will start outputting to that newly activated sheet!

If you're always working offSheet1, then you can use that object reference to qualify these calls, so that no matter what the user does while the code is running, it will always be working with the sameSheet1 object reference:

Sheet1.Columns("A:F").Select

Ideally you would give that worksheet a meaningful name, e.g. if its tab is named "Results", you could set the sheet'scode name toResultSheet, and refer to it like this in code:

ResultSheet.Columns("A:F").Select

Now,Select,Activate, and working with theSelection aren't particularly efficient ways of working with a worksheet. It's typically what less experienced VBA programmers do though, because that's how themacro recorder works.

Why are weselecting columns A:F here?

'Apply format text on cells, and centre it.'Change format to textColumns("A:F").SelectWith Selection    .HorizontalAlignment = xlCenter    .NumberFormat = "@"End With

This could be abstracted into its own procedure:

Private Sub FormatResultSheet()    Dim target As Range    Set target = ResultSheet.Range("A:F")    target.HorizontalAligment = xlCenter    target.NumberFormat = "@" 'otherwise Excel trims leading zeroesEnd Sub

Comments that saywhat the code is doing, are useless and should be removed. The only comments worth typing, are comments that saywhy code is doing what it does (see example above).

This isn't needed at all:

'Clear selectionCells(1, 1).Select

Your code shouldn't be bothered with the current selection.

I won't repeat the excellent points in@Raystafarian's answer about this block:

'Define variablesDim a            As IntegerDim b            As IntegerDim c            As IntegerDim d            As IntegerDim i            As IntegerDim boolCtrl     As BooleanDim period       As VariantDim sCrcy        As VariantDim MsgErr       As String

But I'll add mine:

• Again, comments: anybody can see that these are variable declarations; a comment that says "hey look that's a bunch of variables!" is just a distraction.
• Don't align types like this. It's a sheer waste of time and a royal pain to maintain whenever you rename one single variable.
• Declare variables closer to their usage. "Oh but I like seeing all the variables I'm using in a procedure in one place" isn't a valid argument against that guideline. Code objectively reads more fluently when you're not constantly referring back to that list of variables at the top of the procedure. And it's even worse when there's a dozen of them (or more!), and worse even when the procedure does so many things you constantly need to scroll up and down.

Chr(13) gives youone of the two characters Windows uses to encode new lines. The other character isChr(10).

.." & Chr(13) & "..

A more portable (you know, if you ever want to run that macro on a Mac) and generally more readable way of doing this, is to use the built-invbNewLine constant, which generates whatever "new line" characters the OS likes to use.

The more I read, the more it seems you're using comments to say "this chunk of code does X":

'Add headerRange("A1", "F1").Style = "Input"Range("A1", "F1").Font.Bold = TrueCells(1, 1).Value = "Year"Cells(1, 2).Value = "OffSetCurr"Cells(1, 3).Value = "Month"Cells(1, 4).Value = "toEuro"Cells(1, 5).Value = "toDollars"Cells(1, 6).Value = "toPounds"

Every time you have a comment that says "This chunk does(something)", you have a missed opportunity (it's never too late though!) of extracting a private method thatdoes that thing.

Private Sub AddHeaders()    With ResultSheet        .Range("A1", "F1").Style = "Input"        .Range("A1", "F1").Font.Bold = True        .Cells(1, 1).Value = "Year"        .Cells(1, 2).Value = "OffSetCurr"        .Cells(1, 3).Value = "Month"        .Cells(1, 4).Value = "toEuro"        .Cells(1, 5).Value = "toDollars"        .Cells(1, 6).Value = "toPounds"    End WithEnd Sub

• \$\begingroup\$Thank you so much!! I will implement all of your corrections! Thanks thanks thanks!!!\$\endgroup\$

  svacx

  – svacx

  2016-05-12 16:09:14 +00:00

  CommentedMay 12, 2016 at 16:09
• 2
  \$\begingroup\$@svacxpython one thing I didn't mention, is that your code would complete much much MUCH faster if it did all the processing in an array, and then merelydumped that array in a "one-shot write" to the worksheet.\$\endgroup\$

  Mathieu Guindon

  – Mathieu Guindon

  2016-05-12 16:14:18 +00:00

  CommentedMay 12, 2016 at 16:14
• 1
  \$\begingroup\$re: declaring variables close to their use- I agree, but it seemsSO deems the opposite as correct. ¯_(ツ)_/¯\$\endgroup\$

  Raystafarian

  – Raystafarian

  2016-05-12 16:15:25 +00:00

  CommentedMay 12, 2016 at 16:15
• 2
  \$\begingroup\$@Raystafarian it's not about the scoping or initialization; it's about readability.\$\endgroup\$

  Mathieu Guindon

  – Mathieu Guindon

  2016-05-12 16:16:01 +00:00

  CommentedMay 12, 2016 at 16:16
• 1
  \$\begingroup\$I agree, but answers/comment claim it is more readable all at one place. I'm not sure why they think that, but nobody has said otherwise. Other posters hereagree with all at once being standard. Maybe it's just one of thosethings. Personally, I think declaring everything at the top leads a lot of users to using hungarian notation..\$\endgroup\$

  Raystafarian

  – Raystafarian

  2016-05-12 16:19:41 +00:00

  CommentedMay 12, 2016 at 16:19

• vba
• excel
• web-scraping