HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Building in the built-in declarations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
builtdeclarationsthebuilding

Problem

In order for Rubberduck to be able to "recognize" the built-in VBA functions, procedures and objects, I added yet another constructor parameter to my Declaration class:

public Declaration(QualifiedMemberName qualifiedName, string parentScope,
    string identifierName, string asTypeName, bool isSelfAssigned, bool isWithEvents,
    Accessibility accessibility, DeclarationType declarationType, ParserRuleContext context, Selection selection, bool isBuiltIn = false)
{
    _qualifiedName = qualifiedName;
    _parentScope = parentScope;
    _identifierName = identifierName;
    _asTypeName = asTypeName;
    _isSelfAssigned = isSelfAssigned;
    _isWithEvents = isWithEvents;
    _accessibility = accessibility;
    _declarationType = declarationType;
    _selection = selection;
    _context = context;
    _isBuiltIn = isBuiltIn;
}


That constructor is becoming a real mess, but now I can tell built-in declarations from the rest - this way I can know if the user is trying to rename the MsgBox function, and forbid it instead of happily letting the user break their code; as a bonus I can now also map identifier usages to these declarations, so the user can right-click any MsgBox function call, and I can show them everywhere they're using it in their VBA project.

So I proceeded to implement a little helper class that would give me an IEnumerable containing everything the VBA Standard Library exposes.

Because I'm only ever going to need to instantiate these declarations once, I decided to make it a static class, exposing nothing but a Declarations property getter.

At first I had a #region for each predefined module and class, and another for the predefined enums.. but I don't like #region, so I decided to use private nested types instead, and to define the Declaration instances as public static fields, so that I could easily use reflection to fetch all instances from all nested types, like this:

```
///
/// Defines objects for the standard library

Solution

More than 3 input parameters for a method is considered to be a code smell. One could say ok, I have 4 parameters but I can't help it, but 11 input parameters are way too many.

Problems I see with this type of object (Declaration) is, that the code gets error prone and prone to use a lot of copy and paste.

Instead of using a class you should consider to extract the properties and methods to an interface.

Then you should extract the different types of Declaration's by the meaning of the string asTypeName parameter to different classes which implements this interface.

Example:

Instead of

public static Declaration IsNull = new Declaration(new QualifiedMemberName(EmptyModuleName, "IsNull"), "VBA.Information", "IsNull", "Boolean", false, false, Accessibility.Global, DeclarationType.Function, null, Selection.Home, true);


you could use

public static IDeclaration IsNull = new BooleanDeclaration(new QualifiedMemberName(EmptyModuleName, "IsNull"), "VBA.Information", "IsNull",  false, false, Accessibility.Global, DeclarationType.Function, null, Selection.Home, true);


where BooleanDeclaration would look like

Public class BooleanDeclaration : IDeclaration
{
    public BooleanDeclaration(QualifiedMemberName qualifiedName, string parentScope,
        string identifierName, bool isSelfAssigned, bool isWithEvents,
        Accessibility accessibility, DeclarationType declarationType, ParserRuleContext context, Selection selection, bool isBuiltIn = false)
    {
        _qualifiedName = qualifiedName;
        _parentScope = parentScope;
        _asTypeName = "Boolean";
        _identifierName = identifierName;
        _isSelfAssigned = isSelfAssigned;
        _isWithEvents = isWithEvents;
        _accessibility = accessibility;
        _declarationType = declarationType;
        _selection = selection;
        _context = context;
        _isBuiltIn = isBuiltIn;
    }
}


As you can see the number of constructor parameters are reduced by one. The next to remove would be isWithEvents because a Boolean declaration won't ever be with events.

This can be done for all the basic types like Long, String etc.

By adding an overloaded constructor without the ParserRuleContext parameter you can reduce the number of parameters further.

Alos the Selection seems very often to be Selection.Home which should be reflected in another (or the same) overloaded constructor.

Code Snippets

public static Declaration IsNull = new Declaration(new QualifiedMemberName(EmptyModuleName, "IsNull"), "VBA.Information", "IsNull", "Boolean", false, false, Accessibility.Global, DeclarationType.Function, null, Selection.Home, true);
public static IDeclaration IsNull = new BooleanDeclaration(new QualifiedMemberName(EmptyModuleName, "IsNull"), "VBA.Information", "IsNull",  false, false, Accessibility.Global, DeclarationType.Function, null, Selection.Home, true);
Public class BooleanDeclaration : IDeclaration
{
    public BooleanDeclaration(QualifiedMemberName qualifiedName, string parentScope,
        string identifierName, bool isSelfAssigned, bool isWithEvents,
        Accessibility accessibility, DeclarationType declarationType, ParserRuleContext context, Selection selection, bool isBuiltIn = false)
    {
        _qualifiedName = qualifiedName;
        _parentScope = parentScope;
        _asTypeName = "Boolean";
        _identifierName = identifierName;
        _isSelfAssigned = isSelfAssigned;
        _isWithEvents = isWithEvents;
        _accessibility = accessibility;
        _declarationType = declarationType;
        _selection = selection;
        _context = context;
        _isBuiltIn = isBuiltIn;
    }
}

Context

StackExchange Code Review Q#88093, answer score: 4

Revisions (0)

No revisions yet.