patternMinor
A Moses's worthy string splitter
Viewed 0 times
mosesstringworthysplitter
Problem
I am trying to perfect my string splitter's performance, to be more fast, more easy to maintain if someone else reads it and more readable code-wise.
Context ,Scope and Objectives
Where I work we use a sort of "configuration files" to calculate some data warehouse databases.
For example, if you wanted to config a file to calculate the number of supermarkets in a country, the config file would like the below example, where the:
first line is the description/name of the store and the line below is the code for the program to pick up (where 50 = S is the category for the sales, 97 = 01 is the "I've bought it myself" and 183 is the column where the code for the store is stored (in this case 0040).
So the codes for the configuration are mostly the same across all variables, but it always end with an equal sign.
It happens that when a store has more than 10 codes we have to split them manually, it results in a quite of work.
My ultimate goal for this string splitter is for it to be : fast, reliable, easy to maintain /understand and user friendly.
Concerns about code
Being unexperienced with programming, I still have issues using the proper naming conventions. I have been studying the VBA Developers Handbook by Ken Getz and I didn't quite understand the conventions.
Also I feel that I am using a shotgun to kill a ant (apologies for the cringy metaphor).
Code & Logic
-
I've started with a main object where I store the general procedures to make/call:
```
Option Explicit
Private Sub SplitCodes()
Dim inputRange As Range
Dim currentSheetIndex As Long
currentSheetIndex = ActiveSheet.Index
Set inputRange = Application.InputBox("Select single cell.", "Selection", Type:=8)
ExcelOptimization (True)
If ValidateData(inputRange, currentSheetIndex) = True Then
If RunSplitter(inputRange, currentSheetIndex) = True Then
If RemoveCommas(inputRange, currentSheetIndex) = True Then
Context ,Scope and Objectives
Where I work we use a sort of "configuration files" to calculate some data warehouse databases.
For example, if you wanted to config a file to calculate the number of supermarkets in a country, the config file would like the below example, where the:
first line is the description/name of the store and the line below is the code for the program to pick up (where 50 = S is the category for the sales, 97 = 01 is the "I've bought it myself" and 183 is the column where the code for the store is stored (in this case 0040).
Store1
50=S+97=01+183=0040So the codes for the configuration are mostly the same across all variables, but it always end with an equal sign.
It happens that when a store has more than 10 codes we have to split them manually, it results in a quite of work.
My ultimate goal for this string splitter is for it to be : fast, reliable, easy to maintain /understand and user friendly.
Concerns about code
Being unexperienced with programming, I still have issues using the proper naming conventions. I have been studying the VBA Developers Handbook by Ken Getz and I didn't quite understand the conventions.
Also I feel that I am using a shotgun to kill a ant (apologies for the cringy metaphor).
Code & Logic
-
I've started with a main object where I store the general procedures to make/call:
```
Option Explicit
Private Sub SplitCodes()
Dim inputRange As Range
Dim currentSheetIndex As Long
currentSheetIndex = ActiveSheet.Index
Set inputRange = Application.InputBox("Select single cell.", "Selection", Type:=8)
ExcelOptimization (True)
If ValidateData(inputRange, currentSheetIndex) = True Then
If RunSplitter(inputRange, currentSheetIndex) = True Then
If RemoveCommas(inputRange, currentSheetIndex) = True Then
Solution
Boolean
This can be simplified, if you check a boolean, you don't need to check its value
So either start with
or swap it
Personally, I would use
And swap your calls in
In SplitCodes you do it again
I like the nesting instead of using an
Cancel input
You aren't handling a cancel event for
Function calls
It is excellent you're passing everything
Variable declaration
I see no added value in declaring variables on the same line like this-
Additionally, something like
You'll see by putting the constants at the top, you can change them once without needing to find them everywhere in the code.
LastRow
I think you're trying to get the last row here
There is a standard way to find lastRow and lastColumn. That post explains why.
Error handling
Looks like you have the same error handler everywhere, perhaps just declare it in the main sub and use error handling in each function to return a value that will trigger the error handler. Much better than repeating it a lot.
awesome use of vbNullString - it's something a lot of people miss.
Function or Sub
You have a function here to return a boolean, but you also have things happening in the function. Functions should be used when something is returned and subs should be used when something happens. You can probably refactor that into several procedures. The same might be applicable for other functions.
ActiveSheet
Try to avoid things like activesheet and select - it's unclear. Since you're already finding the Sheet Index, you can create a worksheet variable for whatever worksheet you're on and use that instead of activesheet. It goes along with the comments of unqualified arguments.
On Error GoTo
What could go wrong?
I know it's pretty tempting to use the
What if this returns
But, instead of the goto, you can raise the Err and surround it in an
Counting
This here -
You don't reuse
This can be simplified, if you check a boolean, you don't need to check its value
Private Sub ExcelOptimization(ByVal turnState As Boolean)
If turnState = False Then
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
Application.EnableEvents = True
Else
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False
End If
End SubSo either start with
If Not turnState thenor swap it
If turnState thenPersonally, I would use
turnState = true for turning screenupdating and enableevents to true, so it's less confusing:If turnState Then
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
Application.EnableEvents = True
Else
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False
End IfAnd swap your calls in
SplitCodesIn SplitCodes you do it again
If ValidateData(inputRange, currentSheetIndex) Then
If RunSplitter(inputRange, currentSheetIndex) Then
If RemoveCommas(inputRange, currentSheetIndex) ThenI like the nesting instead of using an
If AND AND because you won't run functions unnecessarily.Cancel input
You aren't handling a cancel event for
Set inputRange = Application.InputBox("Select single cell.", "Selection", Type:=8)Function calls
It is excellent you're passing everything
ByVal. However, I see the main sub is Private - how is it called?Variable declaration
I see no added value in declaring variables on the same line like this-
Dim errorMessage As String, stringToFind As String
Dim countRows As Long, countArray As Long
Dim stringPosition As Long, stringCharacterCount As Long
Dim delimiterArray() As StringAdditionally, something like
countArray I would expect to be an array. And it seems you could use a constant for your string finding:Const STRING_TO_FIND As String = "=|#"
Const DELIMITER As String = "|"
Dim errorMessage As String
Dim rowCount As Long
Dim arrayIndex As Long
Dim stringPosition As Long
Dim characterCount As Long
Dim delimiterArray() As StringYou'll see by putting the constants at the top, you can change them once without needing to find them everywhere in the code.
LastRow
I think you're trying to get the last row here
countRows = Sheets(activesheetindex).Range(inputRange, inputRange.End(xlDown)).Rows.CountThere is a standard way to find lastRow and lastColumn. That post explains why.
Error handling
Looks like you have the same error handler everywhere, perhaps just declare it in the main sub and use error handling in each function to return a value that will trigger the error handler. Much better than repeating it a lot.
arrayString = vbNullStringawesome use of vbNullString - it's something a lot of people miss.
Function or Sub
You have a function here to return a boolean, but you also have things happening in the function. Functions should be used when something is returned and subs should be used when something happens. You can probably refactor that into several procedures. The same might be applicable for other functions.
ActiveSheet
Try to avoid things like activesheet and select - it's unclear. Since you're already finding the Sheet Index, you can create a worksheet variable for whatever worksheet you're on and use that instead of activesheet. It goes along with the comments of unqualified arguments.
On Error GoTo
What could go wrong?
I know it's pretty tempting to use the
GoTo, but if you can, try to handle the error instead of waiting for the error. Anticipate what could error and find a way around the error. For instance:countRows = Sheets(activesheetindex).Range(inputRange, inputRange.End(xlDown)).Rows.CountWhat if this returns
0? Will other errors occur down the line? Why not handle it thereIf countRows = 0 then GoTo ErrorHandlerBut, instead of the goto, you can raise the Err and surround it in an
If to pass it down to the handler. Or, create your own custom errors - which I think is probably beyond you right now, it's nearly beyond me, but I've done it.Counting
This here -
stringCharacterCount = stringCharacterCount + 1
stringPosition = stringPosition + Len(stringToFind)
stringLastPosition = stringPositionYou don't reuse
stringPosition before finding it again, sostringLastPosition = 1
stringPosition = 1
stringToFind = delimiterArray(countArray)
Do
stringPosition = InStr(stringLastPosition, ActiveSheet.Cells(counterRow, inputRange.Column), stringToFind, vbBinaryCompare)
If stringPosition > 0 Then
stringLastPosition = stringPosition + Len(stringToFind)
stringCharacterCount = stringCharacterCount + 1
End If
Loop While stringPosition > 0Code Snippets
Private Sub ExcelOptimization(ByVal turnState As Boolean)
If turnState = False Then
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
Application.EnableEvents = True
Else
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False
End If
End SubIf Not turnState thenIf turnState thenIf turnState Then
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
Application.EnableEvents = True
Else
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False
End IfIf ValidateData(inputRange, currentSheetIndex) Then
If RunSplitter(inputRange, currentSheetIndex) Then
If RemoveCommas(inputRange, currentSheetIndex) ThenContext
StackExchange Code Review Q#152783, answer score: 2
Revisions (0)
No revisions yet.