Based on your 'final version' I figured I would offer some advice. While the great advice offered by Mat's Mug and Vityata seem to have helped you, it also seems as though you are still not quite there. First code, then explanations:
' Avoid prefixing constants with vb. This has special meaning since vbSomeEnum is used to denote an enum
' that is generally used within functions, subs, or classes.
' The naming convention for constants is SOME_FOO where all words are fully capitalized and separated by an underscore.
' Additionally, there is a function for getting these strings. See below:
' Double Quote - Chr(34) - "
' Two Double Quotes - Chr(34)&Chr(34) - ""
' Single Quote - Chr(39) - '
Public Const vbQuadrupleQuote As String = """"""
Public Const vbDoubleQuote As String = """"
Public Const vbSingleQuote As String = "'"
' Ideally, denote your subroutines as 'Public' or 'Private'. This allows your code to be more explicit.
' Additionally, your subroutines should be slightly descriptive. What formulas are you capturing? What is the method doing?
' CaptureFormulas in itself is quite unexplanatory.
' Use proper indentation.
' The Dim block below can be a code smell. It can make it difficult to see the use of variables
' and it can also make it harder to debug your code. I strongly suggest the advice of 'Declare your variables as close to their first use as possible'
Dim Rng As Range
' Why declare these variables as variant? This in itself is a code smell. Variants should only be used as a last resort (and usually, it is when either
' a function needs to return multiple types, or when you are creating an array.
Dim CurrentColumn As Variant
Dim CurrentRow As Variant
' I dont understand the intent of LastRow as String. It is actually being used as a long. As such, declare as long and use explicit
' string conversion functions if needed.
Dim LastRow As String
Dim RangeString As String
Dim FormulaString As String
' Use a more descriptive variable name. Looking at 'Ws' I assume it is a worksheet, but it is instead a sheet name.
' As such, consider 'TargetSheetName' or 'NameOfOutputSheet' or something similar. In turn, your code will become easier to read.
Dim Ws As String
Dim FinalString As String
' These variable names are also somewhat misleading. FormulaType isnt a type at all. It is actually a 'FillToLastRow'.
' SubOrNot is also not fully clear. It is really 'PrintEntireSubroutine'.
Dim FormulaType As VbMsgBoxResult
Dim SubOrNot As VbMsgBoxResult
' I would strongly recommend setting a Range variable to equal the selection, and then refer to that.
' Reason being, if your user somehow interrupts the code the selection at this line may be different from the selection
' at a later line.
Ws = Selection.Worksheet.Name
' If you are going to denote a title, at least make it useful. '???' should be something like 'Formula Fill Length' or something similar.
FormulaType = MsgBox(Prompt:="Fill formulas to last row?", _
' This would be much better written as:
' If Formulatype = vbCancel Then Exit Sub
' It is much clearer that way
Select Case FormulaType
SubOrNot = MsgBox(Prompt:="Print full subscript?", _
Select Case SubOrNot
' Just a suggestion: the user should be allowed to title the new subscript. If you always have
' 'Sub NewScript()' there will inevitably be a conflict.
Select Case SubOrNot
Debug.Print "Sub NewScript ()" & vbNewLine
Debug.Print vbTab & "Dim Ws as Worksheet"
Debug.Print vbTab & "Set Ws = Worksheets(" & vbDoubleQuote & Ws & vbDoubleQuote & ")"
Debug.Print vbTab & "LastRow = Ws.Cells(Rows.Count,1).End(xlUp).Row" & vbNewLine
Debug.Print vbTab & "With Ws"
For Each Rng In Selection
CurrentColumn = Rng.Column
CurrentRow = Rng.Row
' It looks like you actually need a new variable here to be clearer.
' LastRow isn't being used as lastrow, it is being used as FormulaRow and as such, it is unclear why you would
' change the value of LastRow to equal 'LastRow' or 'CurrentRow' whereas 'FormulaRow' being changed would make more sense.
Select Case FormulaType
LastRow = "LastRow"
LastRow = CurrentRow
RangeString = vbTab & vbTab & ".Range(.Cells(" & CurrentRow & "," & CurrentColumn & "),.Cells(" & LastRow & "," & CurrentColumn & ")).FormulaR1C1="
' This can be a one-liner, and as a result your code would be cleaner
' Debug.Print RangeString & vbDoubleQuote & Replace(Rng.FormulaR1C1, vbDoubleQuote, vbQuadrupleQuote) & vbDoubleQuote
FormulaString = Rng.FormulaR1C1
FormulaString = Replace(FormulaString, vbDoubleQuote, vbQuadrupleQuote)
FinalString = RangeString & vbDoubleQuote & FormulaString & vbDoubleQuote
Debug.Print vbTab & "End With" & vbNewLine
Select Case SubOrNot
Debug.Print "End Sub"
The Routine as a Tool
Before I get into any of my suggestions below, I can't stress enough how important it is for someone to not use a 'tool' like this versus learning how to code efficiently. There are plenty of macro-recorders out there, and its nice to have something 'quick and dirty' when you're learning, or when you just dont have time to code it yourself. That said, I cant imagine a scenario in which using a tool like this is more beneficial to the user than learning the 'why' of what the code is using.
That isnt to be harsh, or to say that this project is unusable. I am a huge proponent for writing code to learn how to code. In itself, this project has taught you about coding more than not writing it would have. Does that justify using this instead of continuing to develop as a programmer? Certainly not. In fact, I would give yourself, at most, three months before this project is dead. If, at the end of the next three months, you find this tool still useful then you are doing something wrong.
Particularly, you are not yet using arrays for your loops, and as a result you are directly referencing the sheet. I can guarantee that, for larger loops, this is costing you time. For a bit of perspective, I can loop through 60k rows of data in a matter of seconds (if that). Try doing that with a range reference...well I would recommend not wasting your time.
I can't stress enough: use this project as a learning tool, but not as a practical means of writing code well.
Most of the problems with your code are really stylistic choices, or decisions you've made that you just didnt have enough experience to know why they're bad decisions. Whenever I review code, I look for the things I used to do (and that were holding me back) but I just didnt know it yet.
First: naming conventions. Spend some time learning them, and learning why they exist.
- A constant (
Const) should follow the UPPER_CASE style. This makes it really easy to see when the constant is then used within the code.
- Special prefixes (
vb, etc.) hold special meaning, and should not be used within variable names. While it may make it look nicer, it will also make it more difficult for an outside reader to determine why the supposed
vbSomeFoo is acting strangely, and they will assume that it is a bug within the language. They will not innately know it was a bug you introduced.
Use existing functions/code where possible. This not only reduces the risk of bugs, but it can, at times, be more efficient. For example,
Chr(34) denotes a
" and can be used instead of introducing an entirely new variable. This also makes your code easy to read. When I, as a reader, see
Chr(34) I know what it is, whereas if I see
vbDoubleQuote I have to look for its meaning.
Indent your code properly. I don't think this point can be over-stressed yet it doesn't always sink in properly. If you ever have code that is touching the window, aside from the Sub/Function declarations and Line Labels, something is wrong. For example:
Dim Bar as Baz
Is much harder to read than:
Dim Bar as Baz
Especially when you have:
Dim Bar as Baz
Dim Baz as Foo
While your code isnt hugging the left-hand side of the window, there are lines that, by their indentation, seem like they are actually procedure declarations (Sub Foo's) and not declarations within a procedure.
This gets its own header because it is something often overlooked. It wasn't long ago when, like you, my Dim blocks were like the glossary at the end of a textboox chapter. At times, I was proud of them. "Look at how much I must be accomplishing with this sub. I have so many variables declared!" "My Dim block looks so nice, and uniform. Surely my code is organized." Hyperbole, of course, but I truly was proud of my code. Then came along Mat's Mug to trample all over it (indirectly). I saw his advice about how they can actually be ineffective, ugly, and work against code cleanliness. I decided to give his way a try, and sure enough my code went from "Okay" to "Much Better".
Why are they bad? First, they encourage lazy coding. When you are declaring all your variables at the top, there is no need to question 'Do I really need this here?' and further 'Should this actually be a function?'. These two steps are vital to cleaner code. Even further, they make it more difficult to see when a variable is declared but unused, or when the name doesnt fit the function.
In short, declare your variables as close to their first use as possible, and as a result you should see an improvement in your code.
Variant - The VBA Programming Menace
Variants can be used effectively, and they do have their place within code, so don't think I just write them off completely. I have a few procedures where I use Variants like:
Private Function GetFoo(ByVal InputBar as String) as Variant
If InputBar <> vbNullString Then
GetFoo = 10
GetFoo = vbNullString
What this allows me to do (primarily for data) is use an empty value if the input condition is invalid (null) while still returning a numeric (or some other non-string) if the input is valid. Likewise:
Dim SomeArray as Variant
SomeArray = SomeRange.Value
This allows me to convert a range to an array, and then use those values more efficiently. Variants do have their place when used properly, but they shouldn't be used out of laziness.
For example, in your code,
CurrentColumn (which should be
CurrentRow (which should be
CurrentRowIndex) are used as numerics (and as such, should be Longs) but are declared as Variants. I assume, it is because they are then implicitly (without explicit direction) converted to strings, but even this doesn't justify the use of variants.
You could do something like:
Dim CurrentColumnIndex as Long
CurrentColumnIndex = SomeNumeric
SomeString = CString(SomeNumeric)
SomeLongerString = Chr(34) & CString(SomeNumeric) & Chr(34)
I highly suggest learning Type conversion functions as well.
There is much more to learn beyond what I have highlighted, so don't get discouraged when, at the end of making these improvements, you still have more to learn. The key is to improve with a purpose. I strongly suggest learning about 'Clean Code'. I left notes within your code as well to help you along. The most important part is that you hold yourself accountable for improvement. I can honestly say that my code from three months ago is bad, and my code from six months ago is repulsive. If you can look back and not see the flaws within your code, you arent pushing yourself enough to improve.
Best of luck!