7
\$\begingroup\$

My goal is to prepare the Excel file (see the download link at the end) I would share with my non-technical colleagues, so that they enter some data into it and then I was able to automatically generate CSV without having to manually change format of dates, leading zeros and in other special cases (this CSV I will later use to test another app).

This is my first program in VBA. I understand I did a lot of stupid things, and I would love to be humiliated... But with reasons, so I can improve. I am happy that this implementation allows me to achieve my goal so far, but I think it might be unstable in case if my colleagues try harder (unconsciously) to break it.

In the code below I am trying to change all cells to text format, keeping their values as they are displayed and replacing all the empty cell values by - character "-". Also, I try to remove the duplicated double quotes which might appear.

Please, tell me, how would you improve it to preserve the behavior and make it more transparent? Would you use such tool and why?

Sub PrepareForCSV()

Call CreateBackup
Dim cell1 As Range
Set cell1 = ActiveWorkbook.ActiveSheet.UsedRange
'PART 1:
'Goal: to accomplish setup described here - https://stackoverflow.com/questions/165042/stop-excel-from-automatically-converting-certain-text-values-to-dates

For Each cell1 In cell1
'The check below would help, but it does not work because of Excel internal behaviour
' If (InStr(1, cell1.Value, "=") = 0) Then
'    'cell1.Value = "=" & cell1.Value
'    cell1.NumberFormat = "@"
'inserting - sign to avoid blank cells within UsedRange
'if the cell is empty and format is not text
If cell1.Value = "" And cell1.NumberFormat <> "@" Then
    cell1.Value = "=" & """" & "-" & """"
    cell1.NumberFormat = "@"
'if the cell is empty and format is text
ElseIf cell1.Value = "" And cell1.NumberFormat = "@" Then
    cell1.Value = "-"
    cell1.NumberFormat = "@"
'if the cell starts with double quote followed by = sign and format is not text
 ElseIf (InStr(1, cell1.Value, """=") = 0 And cell1.NumberFormat <> "@") Then
    cell1.Value = "=" & """" & cell1.Value & """"
    cell1.NumberFormat = "@"
'if the cell starts with double quote and format is not text
 ElseIf (InStr(1, cell1.Value, """") = 0 And cell1.NumberFormat <> "@") Then
    cell1.Value = "=" & """" & cell1.Value & """"
    cell1.NumberFormat = "@"
 Else
'probably the code bellow is not needed:
'    If cell1.Value <> "" Then
''        cell1.Value = "=" & """" & cell1.Value & """"
''        cell1.NumberFormat = "@"
'    Else
'        cell1.Value = "=" & """" & "-" & """"
'        cell1.NumberFormat = "@"
'    End If
 End If
Next

'PART 2: replace all unwanted characters
Dim fnd As Variant
Dim rplc As Variant
'replace characters which may cause errors when csv is imported
fnd = ":"
rplc = "!"
  ActiveWorkbook.ActiveSheet.Cells.Replace What:=fnd, Replacement:=rplc, _
    LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
    SearchFormat:=False, ReplaceFormat:=False
 'replace duplicated double quotes
  ActiveWorkbook.ActiveSheet.Cells.Replace What:="""""", Replacement:="""", _
    LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
    SearchFormat:=False, ReplaceFormat:=False
  'replace duplicated double quotes for the second time, in case if they are still there
  ActiveWorkbook.ActiveSheet.Cells.Replace What:="""""", Replacement:="""", _
    LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
    SearchFormat:=False, ReplaceFormat:=False
End Sub

The entire package for review and testing (I separated macros from XLSX for security reasons - you can merge them on your machine) - workbook with supporting macros, mainly backup-restore sheet capacity which allows to play with PrepareForCSV with ease leveraging the backup capacity - with all the credit to @aevanko for his answer https://stackoverflow.com/a/6993228/4778011

\$\endgroup\$
  • 2
    \$\begingroup\$ Hi! Welcome to Code Review. Good job on your first question! \$\endgroup\$ – TheCoffeeCup Jun 27 '16 at 22:01
5
\$\begingroup\$

Overall, your first program in VBA looks pretty good. I've made some modifications below to illustrate a few points that may help you in the future.

  1. Using Call to call the CreateBackup sub is not necessary and is generally not used much anymore. (However, on a single line like this, I've seen it used to more clearly show that another sub is performing work.)
  2. Indenting your code may have gotten messed up when you posted here to SO, but the code below has each logical level indented to highlight where the different logic paths go.
  3. You re-used cell1 for both the UsedRange and the individual cell value within your loop. That's a big no-no. Use specific names to clearly note what each variable refers, such as thisCell and allCells.
  4. Nearly all the references within your first loop are to thisCell. In this situation, encapsulate the code in a With block. This compiles to a common variable reference without having to reevaluate the fully specified name each time. It also gives you somewhat faster executing code.
  5. Separate your logic into levels to make it easier to follow. In your case, when the cell value is empty and when it's not. Then again when the NumberFormat is not text. It's easy to get lost in a single If statement that has many different parts.
  6. Finally, you're making multiple character replacements at the end and you might encounter other character-pairs that need replacing too. To make your code more easily expandable, the find and replace characters can be added to arrays and accessed in a loop. Only the array assignment would change then.
Option Explicit

Sub PrepareForCSV()
    CreateBackup
    Dim thisCell As Range
    Dim allCells As Range
    Set allCells = ActiveWorkbook.ActiveSheet.UsedRange
    '--- PART 1:
    '    Goal: to accomplish setup described here
    '          http://stackoverflow.com/questions/165042/stop-excel-from-automatically-converting-certain-text-values-to-dates

    For Each thisCell In allCells
        With thisCell
            '--- inserting - sign to avoid blank cells within UsedRange
            '    if the cell is empty...
            If IsEmpty(.Value) Then
                If .NumberFormat <> "@" Then
                    '--- if the format is not text
                    .Value = "=" & """" & "-" & """"
                    .NumberFormat = "@"
                Else
                    '--- if the format is text
                    .Value = "-"
                    .NumberFormat = "@"
                End If

            ElseIf .NumberFormat <> "@" Then
                '--- if the cell isn't empty and not formatted as text
                If InStr(1, .Value, """=") = 0 Then
                    '--- starts with "=
                    .Value = "=" & """" & cell1.Value & """"
                    .NumberFormat = "@"

                ElseIf InStr(1, .Value, """") = 0 Then
                    '--- starts with "
                    .Value = "=" & """" & cell1.Value & """"
                    .NumberFormat = "@"
                Else
                    '--- do nothing?
                End If
            Else
                '--- do nothing?
            End If
        End With
    Next

    '--- PART 2: replace all unwanted characters
    Dim fnd() As Variant
    Dim rplc() As Variant
    Dim i As Integer

    '--- replace characters which may cause errors when csv is imported
    fnd = Array(":", """""", """""")
    rplc = Array("!", """", """")

    For i = 1 To UBound(fnd, 1)
        ActiveWorkbook.ActiveSheet.Cells.Replace What:=fnd(i), Replacement:=rplc(i), _
                                                 LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
                                                 SearchFormat:=False, ReplaceFormat:=False
    Next i
End Sub
\$\endgroup\$
  • 2
    \$\begingroup\$ Nice answer. IMO the simple fact that CreateBackup starts with a verb should be enough to say that's a method call - the explicit Call syntax/statement is indeed obsolete/deprecated since the advent of implicit procedure calls a long, long time ago. =) \$\endgroup\$ – Mathieu Guindon Jun 28 '16 at 18:41
  • \$\begingroup\$ @ PeterT, very helpful! I will follow all of these guidelines, especially thisCell and allCells as in many examples on the web this difference is not pointed out. The arrays syntax was tricky for me, now I understand. I will mark this as an answer after I thoroughly test the new code and come up with corrections (if any)! \$\endgroup\$ – Zaplatki Jun 28 '16 at 23:10
  • \$\begingroup\$ Dear All! The only questions to the community which remain: 1) what do you think about the goal itself (use VBA to prevent Excel from corrupting the data)? 2) What do you think about the idea we implemented to accomplish it (intervening into Excel's algorithm to store strings in cells by adding/removing special characters basing on our understanding:to make Excel to "think" the data should not be changed)? 4) Have you played with the workbook and would you use it for your purposes and why? 5) The quality of implementation you covered, I see the areas for improvement and will update it ;-) \$\endgroup\$ – Zaplatki Jun 28 '16 at 23:17
  • \$\begingroup\$ @PeterT - excellent. I noticed that you are iterating through the cells. would you know if there a way of selecting all blank cells and applying formatting changes without iterating in Excel. \$\endgroup\$ – BKSpurgeon Jun 28 '16 at 23:21
  • \$\begingroup\$ @BKSpurgeon - you could try set allBlankCells = ActiveSheet.Range("A1:D100").SpecialCells(xlCellTypeBlanks), then allBlankCells.NumberFormat = "0.00" or any other cell formatting you wish to apply. \$\endgroup\$ – PeterT Jun 29 '16 at 13:56
3
\$\begingroup\$

Small tips:

Don't repeat the wheel

I'd recommend using existing Excel functions as much as possible like: IsEmpty( value ) - this checks whether a cell is empty.

Don't repeat yourself

I also notice you are testing for blanks in the first if statement and in the next elseif statement. Do not repeat yourself twice. DRY. If you make a change and you forget the second one, then there will be bugs down the line.

Use sensible names

Dim cell1 As Range
Set cell1 = ActiveWorkbook.ActiveSheet.UsedRange

If you read this in 6 months will you know what cell1 is? You should be able to understand what things are without reading the comments. Make it easy for yourself and others too. Use sensible names.

\$\endgroup\$
  • \$\begingroup\$ thank you! Nice tips, I will check the usage of IsEmpty(value) in this scenario and let everyone know if it works. The tip itself is good - I will pay more attention to internal functions! I am considering to select PeterT's answer as it covers more points and it was altogether more detailed and useful I think. But your tips are important and I will also follow them. \$\endgroup\$ – Zaplatki Jun 28 '16 at 23:21
  • \$\begingroup\$ @Zaplatki no problem i had to get to work but i could quickly write quick tips which is helpful even though i could not refactor the entire code \$\endgroup\$ – BKSpurgeon Jun 28 '16 at 23:34

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.