Implicit reference to Application.ActiveSheet

[retailcoder]

This one would be Excel-specific, but the idea is to catch these:

foo = Range("A1").Value

And to replace them with an explicit reference to the active sheet:

Dim sheetReference As Worksheet
Set sheetReference = Application.ActiveSheet

foo = sheetReference.Range("A1").Value

It's still referring to Application.ActiveSheet, but at least it's explicit about it - and future refactorings could easily promote the local variable to a paremeter - see #179

---

Note, this is also an implicit reference to Application.ActiveSheet:

foo = Cells("A1").Value

[rubberduck203]

It's got my blessing. :+1:

[retailcoder]

To further complicate matters...

The inspection's quickfix should reuse the generated sheetReference throughout the procedure, not just at the issue site; it's not uncommon for a procedure to refer to ActiveSheet in several places. The fix should be "procedure-scope" ...heck, if possible, the inspection itself should be "procedure-scope".

[robodude666]

Would be nice to provide the option to:

• Provide the name for the quickfix variable.
• Take advantage of the With statement if there are multiple uses in a code block, as long as it doesn't break an existing With.

What about cases where a non-generated variable exists but isn't used:

Dim ActivesheetReference As Worksheet
Set ActivesheetReference = Application.ActiveSheet

foo = Range("A1").Value

Would the quickfix be smart enough to reuse the existing ActivesheetReference ?

[rubberduck203]

I don't really know how I feel about using a With statement there. There are lots of good reasons to avoid them in general.

[retailcoder]

@robodude666 thanks for your feedback! Providing the name should be possible, reusing the dialog for the Rename refactoring (and modifying it to validate the entered name so that user can't enter a name that's already in-scope ...wait, that would be nice to implement for the rename refactoring itself, too).

Using an unused variable declaration is an interesting edge-case that's certainly possible to implement, perhaps by detecting whether there's already a declared variable in-scope that's assigned to ActiveSheet but never referenced, and declared before the line where the implicit reference is located... but that quickly becomes pretty complex and bug-prone. Not saying it's impossible, just that perhaps the first version of this inspection should try to be accurate first, then made smarter! :wink:

I agree with @ckuhn203 about the With statement - they're already quite hard to handle; the parser knows about them, but the symbol table (coming in 1.3) isn't really interpreting the child member calls as such - we/I need to refactor that shitty code and implement some Declaration ResolveReference(IdentifierReference) method in the IdentifierReferenceListener class before that can happen.

So, suggestion noted, but we'll start by making it work - then we'll make it smart :+1:

[robodude666]

@ckuhn203 Could you please point me towards some articles describing why they should be avoided? I haven't heard of such a thing before.

[retailcoder]

@robodude666 it's a readability thing. Correct usage of the With statement witholds the object reference, which lives and dies with the With block, like so:

With foo = New Bar

End With 'foo instance is terminated here

By using With against a variable that has a reference outside of it...

Dim foo As New Bar
With foo

End With 'foo instance is still alive

...you're just avoiding to type the identifier... which IntelliSense already facilitates.

And then things can easily get out of hand when these statements are nested - the find all references feature (and by ricochet, rename and all usage-related code inspections) is actually going to have a very hard time figuring these ones out, until a proper ResolveReference method is implemented.

[retailcoder]

Also worth reading: http://stackoverflow.com/a/18288030/1188513

[robodude666]

@retailcoder I disagree. The With statement isn't meant to be used for reference control. It's meant to make code more readable:

With...End With Statement (Visual Basic)

Executes a series of statements that repeatedly refer to a single object or structure so that the statements can use a simplified syntax when accessing members of the object or structure. When using a structure, you can only read the values of members or invoke methods, and you get an error if you try to assign values to members of a structure used in a With...End With statement.

Based on Microsoft's example is:

Private Sub DoSomething()
    Dim theCustomer As New Customer

    With theCustomer
        .Name = "Coho Vineyard"
        .URL = "http://www.cohovineyard.com/"
        .City = "Redmond"
    End With

    With theCustomer.Comments
        .Add("First comment.")
        .Add("Second comment.")
    End With

    theCustomer.DoSomething()
End Sub

That's a heck of a lot nicer to look at than:

Private Sub DoSomething()
    Dim theCustomer As New Customer
    theCustomer.Name = "Coho Vineyard"
    theCustomer.URL = "http://www.cohovineyard.com/"
    theCustomer.City = "Redmond"
    theCustomer.Comments.Add("First comment.")
    theCustomer.Comments.Add("Second comment.")
    theCustomer.DoSomething()
End Sub

[retailcoder]

@robodude666 I'm not saying With is evil - just that it's often abused, and that it has side-effects that many don't know about.

When it makes sense to use it, use it. When it doesn't, don't.

I agree that initializing properties of a new object makes sense in a With statement. But wrapping an arbitrary number of calls to the various members of a Range may not always do - that's highly depending on context, and whether it improves or decreases readability is also somewhat subjective.

Also notice how the MSDN article specifically avoids nesting With .Comments under the With theCustomer block - which is something the language allows and that we're going to have to deal with when locating/resolving identifier usages.

My point of view is, if you're seeing it as an initialization syntax, you're doing it right; if you're seeing it as some .net-ish using block, you're also doing it right ...but if you're seeing it as a mere shortcut for typing fewer characters when coding, you're doing it wrong.

[retailcoder]

:question: Perhaps there could be an inspection that locates multiple successive calls into the same object variable, and suggests to wrap them inside a With block?

[rubberduck203]

Here's an example of abusing a With block and a typical example of its use with late bound code. I've seen many MSDN articles using this pattern and it's detrimental. It's suggested to use this way so that the With handles the scope and garbage collection of COM objects.

With CreateObject("Library.Foo")
     .Name = "baz"
     .Execute
End With

Now, our Foo object gets released when the With block terminates. This is it's intended use, so what's wrong with it? Step through the code. You can't inspect this with the Locals window or a watch. You can't even inspect it with the immediate window unless you put a Debug.Print statement in there.

And this is its recommended usage.

You can get around it the way RetailCoder did, but now the object remains in memory until the variable goes out of scope, defeating the purpose. Add to this the complication of nested Withs and the fact that I often see this done:

With someWorksheet
    Cells.(1,2) = "foobarred"
End With

And we have a recipe for buggy code. (In case you don't see it, that snippet references the ActiveSheet when it's intended to access someWorksheet.)

There's a reason this feature didn't make the cut for C#.

[robodude666]

The MSDN article does provide a nested usage example:

With theWindow
    With .InfoLabel
        .Content = "This is a message."
        .Foreground = Brushes.DarkSeaGreen
        .Background = Brushes.LightYellow
    End With

    .Title = "The Form Title"
    .Show()
End With

However, it appears I wasn't clear on what I was proposing. All I'm asking is to support turning:

val = Cells(10, 10).Value
data = Range("A1:B2").Value
data1 = Range("C1:D2").Value
data2 = Range("E1:F2").Value

into:

With Application.ActiveSheet
    val = .Cells(10, 10).Value
    data = .Range("A1:B2").Value
    data1 = .Range("C1:D2").Value
    data2 = .Range("E1:F2").Value
End With

instead of:

Dim ASheet As Worklsheet
Set ASheet = Application.ActiveSheet

val = ASheet.Cells(10, 10).Value
data = ASheet.Range("A1:B2").Value
data1 = ASheet.Range("C1:D2").Value
data2 = ASheet.Range("E1:F2").Value

:smile:

[retailcoder]

@ckuhn203 @robodude666 well I'm not against a "wrap successive member calls in With block" feature/inspection - but the goal of this inspection here is to eliminate implicit references to ActiveSheet - perhaps both fixes can be implemented, and user decides which one to apply?

[retailcoder]

@ckuhn203 the purpose is also to make VBA programmers' lives easier, by automating tasks that would otherwise be manual and tedious. I have no objections to having a feature that wraps successive member calls into a With block.

[rubberduck203]

You're right. I've forgotten the reason... My apologies.

[rossknudsen]

OK, I'm leaving the With block tangent alone and just adding my opinion to the topic of implicit referencing of ActiveSheet etc.

I reckon that you don't worry about the refactoring of such items, at least initially. Just create a warning and provide a link to an article which clearly explains what is being done. My justification is that this kind of 'mistake' is likely to be made either by amateur programmers or experienced programmers who have made an oversight.

Amateur programmers need to be educated (hence the link). Experienced programmers just need a pointer to the location where they can fix the problem themselves. If they still want to use implicit references, they can disable the warning in the settings.

[retailcoder]

Closing as a dupe of #385; the two are very closely related anyway.