Skip to main content
7 votes
Accepted

Add Default Rubberduck VBA Folder Annotation to VBProject.VBComponents

HasFolder = InStr(.Lines(1, .CountOfLines), Chr(39) & "@Folder") That condition can technically be True in ...
Mathieu Guindon's user avatar
6 votes
Accepted

Rubberduck Test Module Creation

Only targeting _moduleInit and _methodInit IMO the use of string.Concat with many ...
Heslacher's user avatar
  • 50.4k
6 votes
Accepted

Implementing a class hierarchy and instancing the base class in VBA

If Not DebugMode Then Resume CleanExit Else: Stop: Resume Avoid using the : instruction separator in code. That's good for ...
Mathieu Guindon's user avatar
5 votes

Fighting f̶i̶r̶e̶ regex with f̶i̶r̶e̶ regex

Validation A public method should validate its method parameters. It doesn't matter if it is only used inside one project like RubberDuck or if it is used by other ...
Heslacher's user avatar
  • 50.4k
5 votes
Accepted

Fighting f̶i̶r̶e̶ regex with f̶i̶r̶e̶ regex

Ridiculously Large Bug Okay this is a tiny bit embarrassing, but apparently I'm an idiot. Remember when I said I recently had this in CS class? Well apparently I hadn't been paying attention. In the ...
Vogel612's user avatar
  • 25.2k
5 votes
Accepted

Code Explorer Commands

CodeExplorer_ImportCommand ...
RubberDuck's user avatar
  • 30.8k
5 votes
Accepted

Code Explorer UI

I don't like this: ...
Mathieu Guindon's user avatar
5 votes

Tick. Tick. *breathe* BOOM! - Setting up real, stable asynchronous callbacks with WinAPI Timers in VBA

First I'd like to say that this is impressive work, overall pretty squeaky clean... despite the adjustments needed to make it build on x64 :) One enhancement I can see in terms of readability, would ...
Mathieu Guindon's user avatar
5 votes
Accepted

OOP Dependency inversion principle VBA

An observation on your class constructors. At the moment you are using public properties to allow you to set the value of properties of a class after it has been created. You can take this to the ...
Freeflow's user avatar
  • 645
5 votes

Rigorously checking whether 2 floating point numbers are close in VBA

I admire the fact that you took the time to research and implement the edge cases for NaN and +-Inf. I think most people would just write an On Error Resume Next ...
Cristian Buse's user avatar
5 votes
Accepted

Produce date range string from a database table

Regarding the points of interest you identified: Using either one or two queries to get your data can be acceptable so long as it meets your performance criteria in actual use. However, I am going ...
BZngr's user avatar
  • 1,217
4 votes

Wrapping COM objects with IDisposable

Just a flying review: Have you tested this? The equality code isn't at all right. Firstly, never override == and != without ...
RobH's user avatar
  • 17k
4 votes
Accepted

Wrapping COM objects with IDisposable

There is two things that caught my attention. Every public method / property of all implementing classes needs to call ThrowIfDisposed(). Always. That's the ...
Vogel612's user avatar
  • 25.2k
4 votes

Prettifier isn't pretty

Not knowing anything about RubberDuck, I'm a little frightened by the necessity that the CodeString come from a particular ...
VisualMelon's user avatar
  • 7,431
4 votes
Accepted

BattleShip Grid: Classes and Tests

Based on my first not too thorough read, this looks rather nice. I currently only have two points of critique. The first point is the lack of explicit interfaces. I think both the ...
M.Doerner's user avatar
  • 1,510
4 votes

Constructor-injecting a truckload of commands

After rethinking the problem, I got another idea. Because it is fundamentally different, I'll post it in a separate answer. The commands are injected to the view model, exposed by properties and ...
JanDotNet's user avatar
  • 8,538
4 votes

Enhancing autocompletion for VBA code

This review isn't definitely worth 200 extra points because I think in order to write a decent one, you have to pretty well understand how this code together with VBA works or even try to run it. I ...
t3chb0t's user avatar
  • 44.3k
4 votes
Accepted

Managing Excel Tables (ListObjects) with OOP Approach

On the whole nice work! This is actually a problem I've looked at in the past, and as a result I'd like to review the approach you've taken and the API/ way users can interact with your code. As I ...
Greedo's user avatar
  • 2,445
4 votes
Accepted

Managing Excel Tables (ListObjects) with OOP Approach (Follow up)

I find relying on Sheet.Activate / Sheet.Deactivate to set/unset the sheetTables reference ...
Mathieu Guindon's user avatar
4 votes

Secure ADODB: Unit of Work

I have been silently waiting for you to post this for a while. I was working on something nearly identical to this, and almost posted my stuff a couple of weeks ago, but I wasn't sure of how to hook ...
rickmanalexander's user avatar
4 votes
Accepted

Copy Columns Forward Macro: From Procedural to OOP

The OOP version of the code generated some thoughts around separation of concerns: user interactions and data processing. Single Responsibility Principle(SRP): Every module, class or function in a ...
BZngr's user avatar
  • 1,217
3 votes

Constructor-injecting a truckload of commands

Ninject has a very nice extension called Ninject.Extensions.Factory. It allows you to create factories by specifying an interface. In your case, you could create a factory that creates the commands: <...
JanDotNet's user avatar
  • 8,538
3 votes
Accepted

Enhancing autocompletion for VBA code

Like t3chb0t, I am not familiar with VBA, but I will mention a few things that caught my eye: 1) Lack of documentation. To me it is surprising, that you went through the trouble of documenting a ...
Nikita B's user avatar
  • 13k
3 votes

Prettifier isn't pretty

Because I don't think you will add some binary prettifier, I would rename ICodeStringPrettifier and CodeStringPrettifier to <...
Heslacher's user avatar
  • 50.4k
3 votes
Accepted

IDTExtensibility2 implementation for Rubberduck's entry point

The code that handles the methods OnConnection, OnStartupComplete, OnBeginShutdown and OnDisconnection and their flags is correct. I cannot comment on root composition or NInject. Some things worth ...
Carlos Quintero's user avatar
3 votes

A generic IEnumerator to enumerate COM collections

You rarely need to manually implement enumerators. In your case, you should probably just go with extension methods. For example: ...
Nikita B's user avatar
  • 13k
3 votes
Accepted

A generic IEnumerator to enumerate COM collections

Your ComWrapperEnumerator is generic in two types without needing to. You don't really care about TComCollection aside that it ...
Bruno Costa's user avatar
  • 5,566
3 votes
Accepted

Code Explorer View Models

GetSelectedDeclaration is ugly and a little bit uncomprehensive. CodeExplorerItemViewModel does not implement ...
Bruno Costa's user avatar
  • 5,566
3 votes
Accepted

Abstracting and unit testing lookups in Excel table

First off, nice work overall. It's apparent from the way you set up your interfaces and implementations that you "get it". Given that, most? of this can probably be classified as "nitpicking". I'm ...
Comintern's user avatar
  • 4,192
3 votes

Secure ADODB: Unit of Work

Comment on Transaction DDL. While Microsoft does state that its presence indicates that the backend supports transactions, there is another reference (possibly ...
PChemGuy's user avatar
  • 425

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