Rubberduck: Consider Renaming db, rs, i

[danielleevandenbosch]

Under code inspections, the program asks to consider renaming db, rs, and i. Not that it makes it right, but these are generally accepted place holder variable names for recordset, databases and loop indexes. my question is, is it necessary to call these out in the code inspection prompt?

[retailcoder]

You can whitelist as many identifier names as you wish in the inspection settings dialog, and/or via the use meaningful names inspection results' quickfix dropdown.

The whitelist doesn't come prepopulated, that's for you to decide whether i, db and rs are ok. What's ok for you might not be ok for someone else; RD simply points out identifier names that are short and/or without a single vowel, that aren't whitelisted.

You can also configure the inspection to have a severity level of DoNotShow, which effectively disables the inspection.

[rubberduck203]

It's the belief of the Rubberduck development team that these should be reported to the user for consideration. I suppose you could make an argument for not reporting some commonly used shorthand, particularly i as an indexer, however we believe that it's useful to at least consider that a more meaningful name may be applicable. We report them at an "Info" level because they may not be actual problems. A person needs to look at them and decide for themselves.

I recall a few feature requests that would allow a white list or an Ignore Once annotation, but I'm unsure of the status of either of them. Maybe @retailcoder can speak to the status of those requests.

[rubberduck203]

Oh hey. Look at that. They're already implemented and I'm late to the game.

[retailcoder]

FWIW, IMO rs is only ok for a recordset if it's the only recordset that's used (/that ever will be used) in a given scope. Things get messy when you start having rs1 and then rs2; results or customers or invoices, or literally any other name is more meaningful.

Same for db; if db stands for the only database you're ever using, then fine, it's "the db". Otherwise, productionDb and testDb are more meaningful, or retailDb and wholesaleDb, or whatever.

I used i for For loop variables in the For loops I wrote in BASIC 2.0 on a dusty Commodore-64, where screen estate was rather limited, and every byte counted. Nested loops would typically get j, and k counter variables. Not saying it's wrong, but between this:

For i = 1 To lrow
    For j = 1 To lcol
        .Cells(i, j) = foo
    Next
Next

And this:

For row = 1 To lastRow
    For column = 1 To lastColumn
        .Cells(row, column) = foo
    Next
Next

I can't help but think row is what i really wants to say (and j means column). Recently on Code Review Stack Exchange I saw code like this:

    .Cells(x, y) = foo

A careful eye will notice that x is supplied as a parameter for the row index, and y as a parameter for the column index, which is literally the opposite of how one would typically read (x, y). A maintainer that comes across that code would probably have to read it twice to avoid the trap; wrong code should look wrong (that's also the basis for the Hungarian Notation inspection, which uses the same whitelist as the Use meaningful names inspection; any whitelisted identifier is therefore also a whitelisted Hungarian prefix). The point being, single-letter variables can mean something that's commonly and widely accepted. But they can also be meaningless, or worse, terribly misleading.

So instead of hard-coding a limited set of identifiers that the inspection should ignore because conventions, we leave it up to the user to decide whether they want to rename those, and have RD help them spot every instance.

[danielleevandenbosch]

gotcha that makes a lot of sense.