Hot answers tagged

27

OK, went through the code and gave this a bit of thought over the past couple of days. As far as the implementation goes, I don't see a whole lot that I would change (that you didn't identify in the answer above) other than a couple nit-picky things. First, is the use of the this variable identifier. I couldn't find anything that justifies the naming and ...


25

The problem is that runtime errors in VBA are not exceptions, and error-handling in VBA has very little in common with exception handling. RememberErrLine = Erl() The Erl function is a hidden member of the VBA.Information module for a reason - it returns 0 unless the error occurred on a numbered line. And if you're using line numbers in VBA, you have been ...


23

I like this, a lot. It's.. brilliant. Like .NET String objects, VBA strings are immutable, which means like in .NET, when "the quick brown fox" gets appended with "jumps over" and then "the lazy dog", it's 4 strings that have been generated, and thus the first one got copied 3 times; a VBA StringBuilder class is therefore definitely welcome to any VBA ...


21

IOutput class module (Interface) Looking at how the interface is being used: this.Parent.OutStream.PrintLine output Where output is clearly a String, which makes sense. But the interface's signature doesn't reflect that, and is confusing: Public Sub PrintLine(Optional ByVal object As Variant) Why is the parameter optional? and why is it a Variant? ......


20

Public Function ToString() As String 'Returns a string that represents the current List object. ToString = StringFormat("{0}<{1}>", TypeName(Me), _ Coalesce(this.ItemTypeName, "Variant")) End Function This means the string representation of this List is "List<Variant>" when this.ItemTypeName is empty ...


18

Doing Copy+Paste in your IDE should raise a big red flag and then there should be a neural inhibitor that prevents your left hand from doing it, causing your right hand to move your mouse further down the module and then start typing Private Function... Now the first thing you need to do before you write any code, is "What is it exactly that I need to be ...


18

There are a number of (minor) issues here. Re-raising errors This is rather ugly: Err.Raise Err.Number, Err.source, Err.Description, Err.HelpFile, Err.HelpContext Per MSDN, Err.Raise can "rethrow" an error much cleaner than that: All of the Raise arguments except Number are optional. If you omit optional arguments, and the property settings of the ...


17

This seems extra complexity with no purpose. You take any type variable and automatically convert it to a parameter (this is good). But then something strange happens, you look at the type of the variable and convert that to a string so you can call a function named after the type to do a standard set of options that only change based on the type. Why ...


15

Key Points Each Case block implements formatting functionality for a specific format specifier. Goto statements indicate the function wants to be broken down into several smaller functions. Local variables such as alignmentSpecifier, alignmentPadding, precisionString, precisionSpecifier, formatSpecifier and all others, could all be eliminated if there was a ...


15

Stop. Close Excel. Take a deep breath. Now go and get an actual Database. MS Access. MySQL is free and Open-Source. It doesn't really matter. What matters is that Excel is not the program to be doing this in. At all. 10-Dimensions was already excessive. 31 is a severe case of Over-Engineering. Unless you've been specifically required to include support ...


15

Lowest Hanging VBA Fruit: Public Sub () Application.ScreenUpdating = False Application.EnableEvents= False Application.Calculation= XlManual ... Code ... Application.ScreenUpdating = True Application.EnableEvents= True Application.Calculation= XlAutomatic End Sub Every time you access the worksheet, events trigger, ...


15

Lowest-Hanging VBA Performance Fruit You have With Application .ScreenUpdating = False .EnableEvents = False .DisplayAlerts = False End With But you only have it in force for part of your Sub. If you move this With Application .ScreenUpdating = True .EnableEvents = True .DisplayAlerts = True End With To the very end of your Sub, you should instantly ...


14

I'm assuming that names have been changed to protect the innocent, but just in case they've not been... These are terrible names. All of them. Function names should have verb-noun names and be PascalCased. So instead of CUSTOMFUNCTION should be GetCustomMatch (or something like that. I can't say because you've ommitted any actual names...) Parameters ...


14

I think you've missed the point of having subroutines. The idea is to reduce the problem into small simple steps. Each step of the problem is translated into a subroutine (or a function). Let's start at 30,000 feet in the air. What you want to do is perform an action on a number of worksheets. I'm assuming that you want to work on every sheet in a given ...


14

Decomposition There are redundancies in translating from Array and Collection. Consider these three snippits Dim value As Variant, i As Long 'value is unused? For i = LBound(values) To UBound(values) encapsulated.Add values(i) Next Dim value As Variant For Each value In values result.Add value Next Set result = LinqEnumerable.FromArray(value....


14

Implicit worksheet references Sub CreateGrid() ActiveSheet.Name = "GameBoard" Columns("B:Y").ColumnWidth = 2.14 Columns("A").ColumnWidth = 50 Columns("Z").ColumnWidth = 50 Rows(1).RowHeight = 100 Rows(24).RowHeight = 100 Range("A1:Z1").Merge Range("A1").Interior.Color = vbBlack Range("A24:Z24").Merge Range("A24")....


13

The indentation in TelekomRechnungEinlesen makes it hard to tell where it starts and where it ends. Sub and End Sub define a scope - anything in between should have a Tab: Sub TelekomRechnungEinlesen() Dim lastSetTelekomRow As Long Dim lastSetNovatecRow As Long Dim iterator As Long Dim novatecSumColumn As String Dim mobilenumberSpalte ...


13

Just things that jump out at me: call is redundant. SubName arg, arg, arg5:=arg var = FunctionName(arg, arg, arg5:=arg) Is clearer and less cluttered. This: If bListed = True And bDirectory = True And bRevision = True And bUpdate = True And bProtected = True Then Call SetStatus(True) Exit Sub ElseIf bDirectory = False Then Call SetStatus(...


13

Interesting idea and well-done! Naming I really don't like the names. The names like clsMultiThread is somewhat misleading, since as you noted they don't actually provide any true multi-threading. A unwary user would expect it to work with anything and would be disappointed when all of their queued works painfully completes synchronously. ;) Also, we are ...


12

If Worksheets(Cells(x, "P").Value).Cells(Cells(x, "Q").Value + 4, "C") = "" Then Worksheets(Cells(x, "P").Value).Cells(Cells(x, "Q").Value + 4, "A").Resize(, 10).Insert Shift:=xlDown, CopyOrigin:=xlFormatFromLeftOrAbove Range("F" & x, "H" & x).Copy Worksheets(Cells(x, "P").Value).Cells(Cells(x, "Q").Value + 4, "E")....


12

Error Handling Your error handlers look much cleaner (and ultimately safer) than before. I also like your GetErrorSoure() routine and the "CleanExit:" name of the labels. Very concise. (<-Read as, "I'll be 'borrowing' more of your code'). I see a small issue in your OpenFile() routine. You should probably set OpenFile = False prior to re-raising the ...


12

The only coupling I can see with MSAccess-specific vba is in your exampleCall (why is it Private anyway?): Dim prj As vbProject Set prj = VBE.ActiveVBProject Your code works perfectly fine with Excel vba if you take in a VBProject parameter: Public Sub exampleCall(project As VBProject) If this code lives in a class module called Ext, I can then do this ...


12

Look what I found Dim procedureName As String, lastFound As String Dim procedureBody As String Personally I don't like declaring variables like this, and almost every language allows you to do this in some way or another. I think this is one of those holy war issues though, some programmers like doing this and some programmers say this is bad practice. ...


12

This is by no means a full review, but I did notice something. The way you raise errors could use a little work if you're striving for maintainable code. Err.Raise vbObjectError + 1, "Matrix.Add", "Could not Add matrices: the Rows and Columns must be the same. The left matrix is (" & Me.Rows & ", " & Me.Cols & ") and the right matrix is (...


12

Well, Rubberduck only indicates one issue. Rubberduck Code Inspections - 3/8/2015 12:56:11 PM 1 issue found. Suggestion: Member 'CIV_draw' is implicitly Public - VBAProject.Module1, line 3 Implicit Public Member documentation So, you're off to a good start. You can correct this by explicitly declaring the Subroutine's scope. Public Sub CIV_draw() ...


12

I've a few things you could improve on: For Each c In Range("B2:Y2") c seems like it could use a better name. As I found out, cells shouldn't be used other than for the system function/methods, so avoid naming it that. x = Sheets("GameBoard").Range("A100") y = Sheets("Gameboard").Range("A101") x = x + Vx y = y + Vy Instead of var = var + otherVar, you ...


12

The 3 lowest-hanging VBA performance fruit are: Application.ScreenUpdating = False Application.EnableEvents = False Application.Calculation = xlManual Just make sure to restore them at the end of your sub, and/or if your method encounters an error and stops, else your senior people won't be able to use Excel afterwards and will blame you for ...


12

Here's that conditional, reformatted with line continuations for readability: If Cells(iCntr, 12).Value = "Mule" _ Or Cells(iCntr, 11).Value = "*R1*" _ Or Cells(iCntr, 11).Value = "*R2*" _ Or Cells(iCntr, 7).Value = "*Mule*" _ Or Cells(iCntr, 6).Value = "*Unassigned*" _ Or Cells(iCntr, 12).Value = "PS" _ Or Cells(iCntr, 7).Value = "...


Only top voted, non community-wiki answers of a minimum length are eligible