3
A function should Do One Thing Well.
Yes, this function does have a single stated goal.
But accomplishing that breaks down quite naturally into the goals of:
SmallNumToWords(n), for input 0 <= n <= 999.
NumToWords(n) for non-negative n, which repeatedly breaks out small n and calls the helper function.
Adding several unit tests would be a boon to ...
3
Creating an Unique ID Without Loops
The problem with incrementing the next ID based on the last cell in a column is you will have to sort the data to ensure that it is in ID order. This really can take away from the user experience. A better approach would be to use the WorksheetFunction.Max() to find the current max ID.
"I am very bad at loops"
You ...
3
IMO, you're overcomplicating it. For 99% of the cases, you only need something similar to this:
#If VBA7 Then
'VBA7 declaration style
Private Declare PtrSafe Function GetWindowLong _
Lib "user32" Alias "GetWindowLongA" ( _
ByVal hwnd As LongPtr, _
ByVal nIndex As Long) As ...
3
Option Explicit!
Suggestion #1: Put Option Explicit at the top of the module. Always!
This would have prevented the very embarrassing error of Dict.Add Ary(I, 1), MyClass. Upon which I see that this code would neither compile nor run unless this Class is a default instance.
Function or Sub?
If you are only returning one value, then you can use a Function,...
3
Code Formatting
What certainly could be improved is the indentation of your code for better readability:
Function Do999(ThreeDigits As String)
'Everything after your function header until End Function should be indented
' ...
End Function
Same for further conditional code blocks:
N1 = Left(ThreeDigits, 1): N2 = Right(ThreeDigits, 2)
If N2 > 19 ...
3
Answering my Own question to share with community the Height of stupidity in my code and gradually how over 1 or 2 sleepless nights of testing, the problem was reduce to workable.
First the height of stupidity is the line
If i < .Paragraphs.Count Then
even after knowing well that while working with such giant document interaction with document is ...
2
This code is much easier to read and understand than the last version. This is a massive leap in attaining clean code in a short amount of time.
On Errors
You have the following code:
On Error Resume Next
Set companySheet = Sheets(companyName)
On Error Resume Next
firstColumnContainsNoVisibleCells = sourceTable.AutoFilter.Range.Columns(1)....
2
A couple of review points.
Goto. Using Goto is a real bad smell. In your case, you can remove the spaghetti very easily.
Sub getData(Parsed As Object, company As String, a, wb)
'Parsed contains a collection of dictionaries parsed from a JSON input'
Dim oSheet As Variant
Dim i As Long
Dim j As Long
For Each oSheet In ...
2
You're missing Option Explicit from the top of your code. From the menu at the top choose Tools>Options... to display the Options dialog>Code Settings group>Require Variable Declaration. Make sure that box is checked. Thereafter Option Explicit will be added to the top of all modules mandating that you do Dim fooBar as Range or whatever the appropriate data ...
2
I agree with the OP:
It seems quite complex [...] a fair
amount of redundant lines present that can tidy up [the] code.
I am making an assumption that the code works - although it is hard to tell. I will also assume the line Find first row of table (las row of sheet as data previously cleared) is a typo and meant to be a comment.
I hope the OP will ...
2
The most obvious thing to me is the repetition in the code. I think that addressing the repetition, you can make this game scalable (change the number of guesses, change the number of pegs, change the number of colours).
Public Type GuessArray
ColorOne As GamePieceColor
ColorTwo As GamePieceColor
ColorThree ...
2
The word "hundred" doesn't belong in the Tens array:
It is the only element in the array that starts with a space
It is only accessed at one place in the code
It is never accessed from the place in the code where the other elements are accessed
Therefore it is better to either:
Declare it as a single string variable
Just use the string " hundred" ...
2
Ouch. I say this in a kind way - the code is painful to read.
Use Option Explicit at the top of every module - always!
Properly indent your code - it makes it easier to read, easier to pick up inconsistent logic and easier to maintain.
Arrays, and Excel's ability to convert values of ranges to arrays is your friend in this instance. No need for loops.
...
2
Function TirNoPer360(Cash, Dates As Range, rate As Double)
The function is implicitly Public, implicitly returns a Variant, and implicitly receives all its parameters ByRef; Cash is implicitly a Variant, and the name isn't making it obvious that...
If Opt(Cash, Dates, rate)
...
Function Opt(Caja, Fecha As Range, tasa As Double)
'...
For i = 1 To ...
2
There is not much to review as far as the code goes other than avoid using 3D arrays. The problem with 3D arrays is that you need to know the exact size of the first 3 dimensions because only the last dimension is resizable
I attempted the ReDim Preserve method the values would not be saved to the array.
You may want to post your attempts to use ReDim ...
1
A couple of things:
- Some the non-variant variables data types are not declared
- Avoid using underscores in variable names
- NumToWordsUnsignedInt reads as "Number To Words Unsigned Int" and that just doesn't make sense
- I use IIF() quite often myself but it can take away from the readability of the code
- Ones should be renamed because it contains ...
1
Three initial points:
Always use Option Explicit at the top of the module. Always!
Properly indent your code to help with readability and exposure of some of the logic.
Avoid using .Activate and .Select in your code. It is not necessary unless you want to bring something to the user's attention.
You have d, x, y, z but no real understand what they mean. ...
1
I want to understand this because it seems like an interesting problem, but it's difficult because the parameters are in Spanish and I don't really understand the business domain.
Some things to consider: are you sure there isn't a plugin somewhere that does this calculation?
If not, avoid repeat calculations. Instead of calling Opt four times, store it ...
1
There is no need to instantiate a new instance of MyClass is it already exists and I personally don't like declaring variables inside loops.
Dim ThisClass As MyClass
For I = 2 To UBound(Ary, 1)
If Dict.Exists(Ary(I, 1)) Then
MsgBox "Entry already exists: " & Ary(I, 1)
Else
Set ThisClass = New MyClass
...
1
Since no information was available about the size of source range being copied
Following grey area in the question is assumed as follows
Since 191 Rows X 68 copy X 3 columns take around 10 minutes only (with you code), the range is about 191 Rows X 15 Columns in size
since it has been claimed that code is working Correctly. The Cells of the range (...
1
Important notes:
Proper indenting helps make code easier to read and maintain.
Always use Option Explicit
The first area of performance improvement is to use arrays, not keep referring to Excel objects. As an example - your first loop is:
LastLine = Sheets("MT950").Range("A1").End(xlDown).Row
count = 0
myend = 1
For line = 1 To LastLine
If Sheets("...
1
Off the bat it appears that you are looping through the entire sheet "MT950" twice. Doing this once will improve the speed. Also, the line you said it might be slow around appears twice in your code.
In your first loop you are running two comparisons, one using the If, Like, Then and then another right after using InStr(1, SearchString, swift, 1). These ...
Only top voted, non community-wiki answers of a minimum length are eligible
