Add PredeclaredClassAssignedToVariable inspection

[pchemguy]

What Potential bug: predeclared class instance is assigned to a variable. Potential reason: missed New operator. Automatic fix: Add missing New operator.

Why Predeclared class instances are typically used as class factories. Usually, assigning the predeclared instance to a variable would make little sense.

Example This code should trigger the inspection:

'@PredeclaredId

Public Function Create(ByVal Arg As String) As PredeclaredClass
    Dim Instance As PredeclaredClass
    Set Instance = PredeclaredClass
    Instance.Init Arg
    Set Create = Instance
End Function

---

QuickFixes Should Rubberduck offer one or more quickfix(es) for this inspection? Describe them here (note: all inspections allow for IgnoreOnceQuickFix, unless explicitly specified):

1. Add missing New operator

   Example code, after quickfix is applied:

'@PredeclaredId

Public Function Create(ByVal Arg As String) As PredeclaredClass
    Dim Instance As PredeclaredClass
    Set Instance = New PredeclaredClass
    Instance.Init Arg
    Set Create = Instance
End Function

---

Resources Each inspection needs a number of resource strings - please provide a suggestion here:

• Predeclared class instance is assigned to variable: the name of the inspection, as it appears in the inspection settings.
• Predeclared class instance is usually used as a class factory, and it should make little sense to assign it to a variable.: the detailed rationale for the inspection, as it appears in the inspection results toolwindow's bottom panel.
• Predeclared class {0} is assigned to variable, {0}:the name of the predeclared class: the resource string for an inspection result; if the string needs placeholders, identify what goes in for them (e.g. Variable {0} is not used, {0}:the name of the variable).

[FullValueRider]

Personally I'd write

set Instance = PredeclaredClass.Init(arg)

And consequently I'd suggest that your argument above needs to be reversed so that we get an inspection warning if we use New with a PredeclaredId.

e.g.

set Instance = New PredeclaredClass

should trigger the inspection warning.

[Vogel612]

@FullValueRider I like that idea, but that honestly sounds like a separate inspection that is contingent on having such an Init method (or more generally a method on the Predeclared class that returns a new instance of the class itself)

[retailcoder]

This inspection could be made to also flag redundant variables that just copy the predeclared pointer, for example:

Dim wb As Workbook
Set wb = ThisWorkbook

Or:

Dim ws As Worksheet
Set ws = Sheet1

I like this inspection idea very much! So I'd make it flag any such assignment, regardless of whether the class is user-defined or not - and then a quickfix could offer to add a New operator for user code, and/or to just remove the redundant declaration otherwise (and replace any uses of the redundant variable with the predeclared instance).

[pchemguy]

Personally I'd write

set Instance = PredeclaredClass.Init(arg)

should trigger the inspection warning.

This is wrong! Init is a constructor! Factory is responsible for New'ing the class and is called on the predeclared instance. Constructor is responsible for initialization and must be called on the new instance. That is the reason why we need a factory and constructor. One of them is not sufficient.

And you must use the New operator to create an instance of a class. When the factory pattern is used, New goes inside the factory, which is what my code shows.

[FullValueRider]

Please explain why. As an example here is an example of my current useage. I haven't yet got my head around TSomething and TFactorySomething as explained in one of Mat's blogs. Following feedback I now use Deb( short for debut or debutate) and ReadyToUseInstance instead of Create/Init and Self.

'@Description("Returns a new instance of the Lyst Class. Optionally populates the instance with the result of For Each applied to ipIterable.")
Public Function Deb(Optional ByVal ipIterable As Variant = Empty) As Lyst

    Guard UsePredeclaredIdInstance, Not (Me Is Lyst), i.Location & "Deb"

    If Not VBA.IsEmpty(ipIterable) Then

        Guard InvalidIterable, Types.Group.Iterables.LacksItem(ipIterable), i.Location & "Deb"

    End If

    With New Lyst

        Set Deb = .ReadyToUseInstance(ipIterable)

    End With

End Function

'@Description("For internal use only.  Instantiates a new instance of the lyst class")
Friend Function ReadyToUseInstance(ByVal ipIterable As Variant) As Lyst

    Set ReadyToUseInstance = Me
    Set s.Host = New ArrayList

    If VBA.IsEmpty(ipIterable) Then Exit Function

    Dim myItem As Variant
    For Each myItem In ipIterable

        s.Host.Add myItem

    Next

End Function

[pchemguy]

Ok, "With New" can be used instead of declaring an instance variable within the factory (Deb).

Is there an advantage of using

With New Lyst
    Set Deb = .ReadyToUseInstance(ipIterable)
End With

over

Dim Instance as Lyst
Set Instance = New Lyst
Instance.ReadyToUseInstance(ipIterable)

though?

Well, the former is cleaner in a sense, sure. And helps to avoid the missing New operator issue by using a different pattern.

[FullValueRider]

I suspect the methodology was used to avoid creating an instance variable. I just copied what I thought was a good idea. Nothing wrong with your version though.

[Energeer]

Came across this today. Is there any sort of convention/example for a guard clause that can be written for when a predeclared (factory) class is used as a variable? Particularly for when the factory creates the class as an interface and the variable is the same interface.

I was thinking like having a Boolean member variable in the class which is only true if the factory method is used?

Thanks!