1
\$\begingroup\$

The below code is working I just need a review as there are somethings that I think I might have done wrong. The code will edit the raw data of an excel CSV comma delimited file moving all text using textToColumn by deliminator ";" as all except heather are in column A, formatting into a table and hiding unnecessary columns removing the extra spaces from first 2 rows and saving to the hdd in a specific folder.

Main Sub

 Option Explicit
    Public WS As Worksheet

    Sub editRawData()
    '
    ' edit Macro
    '
    ' Keyboard Shortcut: Ctrl+Shift+I
    '
    Dim myResult As VbMsgBoxResult

        myResult = MsgBox("Are you sure you want to edit this worksheet?", vbQuestion + vbOKCancel + vbDefaultButton1, "Edit worksheet?")
        If myResult = vbCancel Then Exit Sub

    On Error GoTo ErrorHandler
    Application.ScreenUpdating = False

        Range(Cells(1, 1), Cells(Rows.Count, 1).End(xlUp)).Offset(1, 0).Select
        ActiveWindow.FreezePanes = True
        Selection.TextToColumns Destination:=ActiveCell, DataType:=xlDelimited, _
            TextQualifier:=xlDoubleQuote, ConsecutiveDelimiter:=False, Tab:=False, _
            Semicolon:=False, Comma:=False, Space:=False, Other:=True, OtherChar _
            :=";", TrailingMinusNumbers:=True

            Call removeHeatherSpace
            changeProperties WS
            Call SaveAsToFolderPath

        Range(Cells(1, 23), Cells(Rows.Count, 23).End(xlUp)).Offset(1, 0).Select

      Application.ScreenUpdating = True
    Exit Sub
    ErrorHandler:
    Application.ScreenUpdating = True
    Debug.Print Err.Description
    MsgBox "Sorry, an error occured." & vbCrLf & Err.Number & " " & Err.Description, vbCritical, "Error!"

    End Sub

Edit Properties Sub

Private Sub changeProperties(WS As Worksheet)

Dim tableRng As Range
Dim columnsToHide As Range
Dim heatherFormat As Range

Set heatherFormat = Range(Cells(1, 1), Cells(1, Columns.Count).End(xlToLeft)).Columns
Set tableRng = Range("A2").CurrentRegion
Set WS = ActiveSheet

With tableRng.Borders
        .LineStyle = xlContinuous
        .Weight = xlThin
    End With
    With heatherFormat.Font
                      .Name = "Arial"
                      .FontStyle = "Bold"

    End With
    heatherFormat.Interior.ColorIndex = 17
      tableRng.Cells.EntireColumn.AutoFit
With WS

   Set columnsToHide = Application.Union(.Columns("A:F"), _
                                         .Columns("H:O"), _
                                         .Columns("Q"), _
                                         .Columns("S:V"), _
                                         .Columns("Z:AE"), _
                                         .Columns("AI:BD"))
        columnsToHide.EntireColumn.Hidden = True

   End With


     With WS.PageSetup
            .PrintArea = ""
            .PrintTitleRows = ""
            .PrintTitleColumns = ""
            .LeftHeader = ""
            .CenterHeader = ""
            .RightHeader = ""
            .LeftFooter = ""
            .CenterFooter = ""
            .RightFooter = ""
            .LeftMargin = Application.InchesToPoints(0.25)
            .RightMargin = Application.InchesToPoints(0.25)
            .TopMargin = Application.InchesToPoints(0.75)
            .BottomMargin = Application.InchesToPoints(0.75)
            .HeaderMargin = Application.InchesToPoints(0.3)
            .FooterMargin = Application.InchesToPoints(0.3)
            .Orientation = xlLandscape
            .Zoom = False
            .FitToPagesWide = 1
            .FitToPagesTall = False

        End With
    End Sub

Remove space Sub

Sub removeHeatherSpace()


    Dim rngRemoveSpace As Range
    Dim CellChecker As Range


  Set rngRemoveSpace = Range(Cells(1, 1), Cells(2, Columns.Count).End(xlToLeft)).Columns

    rngRemoveSpace.Replace What:=Chr(160), Replacement:=Chr(32), LookAt:=xlPart, SearchOrder:=xlByColumns, MatchCase:=False

    For Each CellChecker In rngRemoveSpace.Cells
        CellChecker.Value = Application.Trim(CellChecker.Value)
        CellChecker.Value = Application.Clean(CellChecker.Value)
    Next CellChecker

    Set rngRemoveSpace = Nothing

End Sub

SaveAs to specific folder

Sub SaveAsToFolderPath()

Dim MyFileName As String
Dim folderPath As String
Dim dateFormat As String

       folderPath = "C:\Users\A\Desktop\M work\DFMS\"
       dateFormat = Format(Now, "dd.mm.yyyy HH-mm-ss AMPM")
       MyFileName = Range("G2").Value

    If Not ActiveWorkbook.Saved Then
        ActiveWorkbook.SaveAs Filename:=folderPath & MyFileName & " - Next Delivery " & dateFormat & ".xlsm"
    End If

End Sub
\$\endgroup\$
2
\$\begingroup\$

To get you started, you have a number of unqualified references within your code. This is particularly puzzling to me since you declare and pass worksheet objects, but you never use this object when determining a reference. For example:

Private Sub changeProperties(WS As Worksheet)

    Dim tableRng As Range
    Dim columnsToHide As Range
    Dim heatherFormat As Range

    Set heatherFormat = Range(Cells(1, 1), Cells(1, Columns.Count).End(xlToLeft)).Columns
    Set tableRng = Range("A2").CurrentRegion
    Set WS = ActiveSheet

This bit of code is a prime example for what I am referring to, and is a good learning opportunity as well. Let's refactor it together.

We will start with the sub declaration. As Mat's Mug will likely point out, you require 'WS as Worksheet' but this declaration is implicitly ByRef which means the object passed will be modified by any later actions on that object. To me, it looks like you are trying to use a Sub here to return a value (which is the purpose of a function). You have two options:

Private Function changeProperties() as Worksheet

And then we could do:

Set changeProperties = Activesheet

Or you could explicitly define the parameter as ByRef

Private Sub changeProperties(ByRef as Worksheet)

Neither of these changes will change the performance of your code, but it will make your code easier to read and understand, and it will start teaching you some coding habits that will be important later. Now, let's tackle that pesky ActiveSheet. I must say, there is no habit that must be broken sooner than Select, Activate, ActiveSheet, ActiveWorkbook, and Selection.

There's good reason why this is a bad habit. First, it makes your code unreliable. While the ActiveSheet could be the correct sheet, we have no way of absolutely guaranteeing. For example:

Sub Foo()
    ThisWorkbook.Sheets("bar").Activate

    ActiveWorksheet.Name = "baz"

End Sub

Nine times out of ten, this procedure will do exactly as we expect it to. It will find the "bar" worksheet within the workbook running the code, and then rename that sheet to "baz". What would happen though if we paused our code after Activating the sheet (perhaps for debugging?) and then a different sheet was activated. Now we have a bug.

This is easy to overcome:

Sub Foo()
    Dim NameChangeTarget as Worksheet

    Set NameChangeTarget = ThisWorkbook.Sheets("bar")

    NameChangeTarget.Name = "baz"

End Sub

Now, we can change sheets all we want but the correct sheet will be modified. This is a prime example of why we must always qualify our ranges. There will be some circumstances where ActiveSheet may be justified. The only instance I have had this happen with is when I am detailing into PivotTables, and getting the output sheet. Even then though, I try to avoid this practice if I have the time.

Next, naming conventions within your code. Overall, you are on the right track, but lets look at a couple areas where you could improve.

WS as Worksheet

This is very common, and I made the same error in naming when I first started off. For a while, it will be unlikely that you have any issues with this. There will come a point though where you are debugging your code, and you have multiple Worksheet variables, and WS just doesnt tell you what you need. Consider useful names that tell you something about what the variable contains such as these:

FormatTarget ' Tells us this is the worksheet being targeted for reformatting
DataOutput   ' Tells us that this is where the data is, and it is an output sheet
OutputDataNeedsFormatting ' This name sucks, but it does the same job as both names above

I am certainly not an expert on naming conventions, so find what works for you. Definitely be mindful of what your names tell your 'reader' though.

The other point worth noting here is how you have names that are inconsistent:

tableRng ' Tells us that it is a range despite it being declared as a range
columnsToHide ' Pretty good name, we know these are the columns being hidden

' On the one hand, I have no idea what 'heather' is. Do you mean header? 
' On the other hand, by the 'Format' I assume that this is formatting, but it is a range.
heatherFormat

Lastly, lets look at this particularly troubling line:

Set heatherFormat = Range(Cells(1,1), Cells(1, Columns.Count).End(xlToLeft)).Columns

   ' First, the name is somewhat confusing, as noted before. We then are using
   ' an implicit call to 'ActiveSheet' since 'Range()' is unqualified
   ' next, we are setting a range based upon two more unqualified 'Cells' references, 
   ' and we are resizing this range to the left until we find data. After all of this, 
   ' we are retrieving the columns and then putting it back into heatherFormat.

Consider something like this instead:

' You can always pass in the sheet as a variable, and get the specific
' sheet from the person running the code when they first call the routine.
With ThisWorkbook.Sheets("SheetName")
' Or With PassedInVariable
    Set headerFormat = .Range(.Cells(1, 1), .Cells(1, .Columns.Count).End(xlToLeft)).Columns
End With

There isn't a big difference here. I have taken an extra step to ensure I have qualified the object I am working with, and I use a With block to do this. As a result, all I need to do is put a period before each object I need to qualify and voila! On bigger projects, With blocks can increase performance, and they make your code much easier to read. Above all else, they get rid of the pesky implicit ActiveSheet calls.

I'll insert a plug for RubberDuck here. Mat's Mug and his team have come up with a great tool that can make this process much easier on you. RubberDuck has a 'Code Inspection' feature that finds all this stuff for you, helps you fix it, and oftentimes tells you why it is wrong. Check it out if you have a chance.

Once you have refactored this code to remove this beginner stuff, you may be interested in using the Split() function, and Variant arrays in order to accomplish the original splitting of your data.

Good luck!

\$\endgroup\$
  • \$\begingroup\$ 's Thank you very much for your feedback sorry about the mistake but I'm just a bigger at this and I'm learning as i go so this stuff is new to me. The other thing I don't get is if you receive all file from external sources how would the code pick up if there will be a different sheet name or sheet number? And then this will need to work on all workbooks that I'm receiving over emails this mean that the code needs to be in the personal macro workbook and then if i use ThisWorkbook it will not work i think because the module is not in that workbook. \$\endgroup\$ – QuickSilver May 8 '17 at 15:07
  • \$\begingroup\$ Also if you know a good material where to read all about this stuff please point me in the right direction. Also just noticed that my code for saveAs is not working properly in the sense that when I try to open the file it's says that the file may be correct or the format is wrong. Tried different formats but still after the save I can't open for someone reason. Thanks for your valuable feedback. \$\endgroup\$ – QuickSilver May 8 '17 at 15:11
  • \$\begingroup\$ @QuickSilver No need to be sorry. I figured you were just learning, and none of the above is meant as a criticism. Almost all of us in the community started by recording our macros through Excel, and then editing them to improve them. I dont expect you to just know this stuff. That's what CodeReview is for, improvement! Here is a post on avoiding select when writing in VBA: stackoverflow.com/questions/10714251/… . Chip Pearson is an amazing resource: cpearson.com/Excel/MainPage.aspx . He has a lot, so be patient as you wade through \$\endgroup\$ – Brandon Barney May 8 '17 at 17:42
  • \$\begingroup\$ I personally use WiseOwl tutorials if I am working on learning new stuff in VBA and this will help if you are a visual learner: youtube.com/playlist?list=PLNIs-AWhQzckr8Dgmgb3akx_gFMnpxTN5 . There's plenty of other stuff out there beyond this, so be patient and practice, and continue using CodeReview. We will always be here to help you continue improving your code. \$\endgroup\$ – Brandon Barney May 8 '17 at 17:43
  • \$\begingroup\$ To answer your question about the code picking up on external sources, I am not completely certain what you mean. There's a number of ways we can 'tell' our code what object we are working with. If you do indeed want the code to target the activesheet (when the user runs the code) then declare a varaible for it, Dim SomeWorksheet as Worksheet and then set it Set SomeWorksheet = ActiveWorksheet. While not ideal, it will at least ensure you are performing your operations on the same worksheet throughout the code. Go through the resources above and you may find other ideas too. \$\endgroup\$ – Brandon Barney May 8 '17 at 17:46

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.