Declaration of bloatedness: the class that knew too much?

Question

I've already expressed concerns about this type's constructor, and I've sort-of* implemented the changes suggested in the answers I got there.

Recently I added even more parameters to that constructor - namely the ParentDeclaration and the Annotations. The Declaration type is absolutely where these things belong. For example when I come across this:

'@ignore VariableNotUsed
Dim foo As Integer

Then the Declaration object representing this foo variable contains an Annotation string that says @ignore VariableNotUsed, and the code inspections use that to know whether or not to raise a flag when References contains no IdentifierReference that isn't an assignment.

So I'm very much torn right now: I've always seen constructor-bloat as a violation of the Single Responsibility Principle... does this class break SRP?

In Rubberduck's API, a Declaration is just about anything: a VBAProject, a class or a standard (.bas) module, a form, the controls on a form, variables, constants, enums, enum members, procedures, functions - everything that has an identifier that can be referenced somewhere in code.

I need to clean up that constructor... it's fairly possible that I need to add more members in the future. It seems to me that everything has a purpose and is in its place. What am I not seeing?

Subclassing to chop off a parameter or two completely sounds like an ineffective way of misusing inheritance to solve the wrong problem. Does it?

Declaration.cs

namespace Rubberduck.Parsing.Symbols
{
    /// <summary>
    /// Defines a declared identifier.
    /// </summary>
    [DebuggerDisplay("({DeclarationType}) {Accessibility} {IdentifierName} As {AsTypeName} | {Selection}")]
    public class Declaration
    {
        public Declaration(QualifiedMemberName qualifiedName, Declaration parentDeclaration, string parentScope,
            string asTypeName, bool isSelfAssigned, bool isWithEvents,
            Accessibility accessibility, DeclarationType declarationType, bool isBuiltIn = true, string annotations = null)
            :this(qualifiedName, parentDeclaration, parentScope, asTypeName, isSelfAssigned, isWithEvents, accessibility, declarationType, null, Selection.Home, isBuiltIn, annotations)
        {}

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

        private readonly bool _isBuiltIn;
        public bool IsBuiltIn { get { return _isBuiltIn; } }

        private readonly Declaration _parentDeclaration;
        public Declaration ParentDeclaration { get { return _parentDeclaration; } }

        private readonly QualifiedMemberName _qualifiedName;
        public QualifiedMemberName QualifiedName { get { return _qualifiedName; } }

        private readonly ParserRuleContext _context;
        public ParserRuleContext Context { get { return _context; } }

        private readonly IList<IdentifierReference> _references = new List<IdentifierReference>();
        public IEnumerable<IdentifierReference> References { get { return _references; } }

        private readonly string _annotations;
        public string Annotations { get { return _annotations; } }

        public void AddReference(IdentifierReference reference)
        {
            if (reference == null || reference.Declaration.Context == reference.Context)
            {
                return;
            }

            if (reference.Context.Parent != _context 
                && !_references.Select(r => r.Context).Contains(reference.Context.Parent)
                && !_references.Any(r => r.QualifiedModuleName == reference.QualifiedModuleName 
                    && r.Selection.StartLine == reference.Selection.StartLine
                    && r.Selection.EndLine == reference.Selection.EndLine
                    && r.Selection.StartColumn == reference.Selection.StartColumn
                    && r.Selection.EndColumn == reference.Selection.EndColumn))
            {
                _references.Add(reference);
            }
        }

        private readonly Selection _selection;
        /// <summary>
        /// Gets a <c>Selection</c> representing the position of the declaration in the code module.
        /// </summary>
        /// <remarks>
        /// Returns <c>default(Selection)</c> for module identifiers.
        /// </remarks>
        public Selection Selection { get { return _selection; } }

        public QualifiedSelection QualifiedSelection { get { return new QualifiedSelection(_qualifiedName.QualifiedModuleName, _selection); } }

        /// <summary>
        /// Gets a reference to the VBProject the declaration is made in.
        /// </summary>
        /// <remarks>
        /// This property is intended to differenciate identically-named VBProjects.
        /// </remarks>
        public VBProject Project { get { return _qualifiedName.QualifiedModuleName.Project; } }

        /// <summary>
        /// Gets the name of the VBProject the declaration is made in.
        /// </summary>
        public string ProjectName { get { return _qualifiedName.QualifiedModuleName.ProjectName; } }

        /// <summary>
        /// Gets the name of the VBComponent the declaration is made in.
        /// </summary>
        public string ComponentName { get { return _qualifiedName.QualifiedModuleName.ComponentName; } }

        private readonly string _parentScope;
        /// <summary>
        /// Gets the parent scope of the declaration.
        /// </summary>
        public string ParentScope { get { return _parentScope; } }

        private readonly string _identifierName;
        /// <summary>
        /// Gets the declared name of the identifier.
        /// </summary>
        public string IdentifierName { get { return _identifierName; } }

        private readonly string _asTypeName;
        /// <summary>
        /// Gets the name of the declared type.
        /// </summary>
        /// <remarks>
        /// This value is <c>null</c> if not applicable, 
        /// and <c>Variant</c> if applicable but unspecified.
        /// </remarks>
        public string AsTypeName { get { return _asTypeName; } }

        public bool IsArray()
        {
            if (Context == null)
            {
                return false;
            }

            try
            {
                var declaration = ((dynamic)Context); // Context is AmbiguousIdentifier - parent is the declaration sub-statement where the array parens are
                return declaration.LPAREN() != null && declaration.RPAREN() != null;
            }
            catch (RuntimeBinderException)
            {
                return false;
            }
        }

        public bool IsTypeSpecified()
        {
            if (Context == null)
            {
                return false;
            }

            try
            {
                var asType = ((dynamic) Context).asTypeClause() as VBAParser.AsTypeClauseContext;
                return asType != null || HasTypeHint();
            }
            catch (RuntimeBinderException)
            {
                return false;
            }
        }

        public bool HasTypeHint()
        {
            string token;
            return HasTypeHint(out token);
        }

        public bool HasTypeHint(out string token)
        {
            if (Context == null)
            {
                token = null;
                return false;
            }

            try
            {
                var hint = ((dynamic)Context).typeHint() as VBAParser.TypeHintContext;
                token = hint == null ? null : hint.GetText();
                return hint != null;
            }
            catch (RuntimeBinderException)
            {
                token = null;
                return false;
            }
        }

        public bool IsSelected(QualifiedSelection selection)
        {
            return QualifiedName.QualifiedModuleName == selection.QualifiedName &&
                   Selection.ContainsFirstCharacter(selection.Selection);
        }

        private readonly bool _isSelfAssigned;
        /// <summary>
        /// Gets a value indicating whether the declaration is a joined assignment (e.g. "As New xxxxx")
        /// </summary>
        public bool IsSelfAssigned { get { return _isSelfAssigned; } }

        private readonly Accessibility _accessibility;
        /// <summary>
        /// Gets a value specifying the declaration's visibility.
        /// This value is used in determining the declaration's scope.
        /// </summary>
        public Accessibility Accessibility { get { return _accessibility; } }

        private readonly DeclarationType _declarationType;
        /// <summary>
        /// Gets a value specifying the type of declaration.
        /// </summary>
        public DeclarationType DeclarationType { get { return _declarationType; } }

        private readonly bool _isWithEvents;
        /// <summary>
        /// Gets a value specifying whether the declared type is an event provider.
        /// </summary>
        /// <remarks>
        /// WithEvents declarations are used to identify event handler procedures in a module.
        /// </remarks>
        public bool IsWithEvents { get { return _isWithEvents; } }

        /// <summary>
        /// Returns a string representing the scope of an identifier.
        /// </summary>
        public string Scope
        {
            get
            {
                switch (_declarationType)
                {
                    case DeclarationType.Project:
                        return "VBE";
                    case DeclarationType.Class:
                    case DeclarationType.Module:
                        return _qualifiedName.QualifiedModuleName.ToString();
                    case DeclarationType.Procedure:
                    case DeclarationType.Function:
                    case DeclarationType.PropertyGet:
                    case DeclarationType.PropertyLet:
                    case DeclarationType.PropertySet:
                        return _qualifiedName.QualifiedModuleName + "." + _identifierName;
                    default:
                        return _parentScope;
                }
            }
        }

        public override bool Equals(object obj)
        {
            var other = obj as Declaration
            if (other == null)
            {
                return false;
            }

            return other.IdentifierName == IdentifierName
                && other.ParentDeclaration == ParentDeclaration
                && other.Selection == Selection
                && other.Context == Context
                && other.Project == Project
                && other.QualifiedMemberName == QualifiedMemberName;
        }

        public override int GetHashCode()
        {
            return string.Concat(QualifiedName.QualifiedModuleName.ProjectHashCode, ProjectName, ComponentName, _parentScope, _identifierName).GetHashCode();
        }
    }
}

Does this Equals implementation comply to the Geneva convention? I feel it's a little bit overkill and could be simplified a bit.

*I said sort-of, because I didn't end up subclassing the Declaration type for every primitive type out there, and instead of making a BuiltInDeclaration subclass, I made a ValuedDeclaration - which I'm using whenever I encounter a Const declaration. For the built-in declarations, this has the benefit that Rubberduck knows what the built-in values are, which can eventually be leveraged for interestingly smart suggestions (although we're not exactly there yet):

ValuedDeclaration.cs

namespace Rubberduck.Parsing.Symbols
{
    public class ValuedDeclaration : Declaration
    {
        /// <summary>
        /// Creates a new valued built-in declaration.
        /// </summary>
        public ValuedDeclaration(QualifiedMemberName qualifiedName, Declaration parentDeclaration, string parentScope,
            string asTypeName, Accessibility accessibility, DeclarationType declarationType, string value)
            : this(qualifiedName, parentDeclaration, parentScope, asTypeName, accessibility, declarationType, value, null, Selection.Home, true)
        {
        }

        public ValuedDeclaration(QualifiedMemberName qualifiedName, Declaration parentDeclaration, string parentScope,
            string asTypeName, Accessibility accessibility, DeclarationType declarationType, string value,
            ParserRuleContext context, Selection selection, bool isBuiltIn = false)
            :base(qualifiedName, parentDeclaration, parentScope, asTypeName, true, false, accessibility, declarationType, context, selection, isBuiltIn)
        {
            _value = value;
        }

        private readonly string _value;
        /// <summary>
        /// Gets a string representing the declared value.
        /// </summary>
        public string Value { get { return _value; } }
    }
}

All other feedback welcome.

Answer 1

Implement IEquatable<T>

Generally speaking, whenever I find myself writing overrides for object.Equals and object.GetHashCode, I also make the type implement IEquatable<T>. It's generally pretty easy to do so by calling the IEquatable<T>.Equals(T) method from the object.Equals(object) override:

public override bool Equals(object obj)
{
    return Equals(obj as Declaration);
}

public bool Equals(Declaration other)
{
    return other != null
        && other.IdentifierName == IdentifierName
        && other.ParentDeclaration == ParentDeclaration
        && other.Selection == Selection
        && other.Context == Context
        && other.Project == Project
        && other.QualifiedMemberName == QualifiedMemberName;
}

Answer 2

You really should group those properties and method together! Right now it's complicated to see what is what because there are some method between properties and properties between methods! (That's just nitpicking)

I thought maybe you could leave some properties assignation to the setter of the said properties, but all your properties are read-only!!

Which means, that solution isn't good.

You could use inheritance like you talked about, but you might end up having as many super classes as you have parameters which might be a worst problem.

Finally, someone talked about the Builder pattern. I might see one problem with this solution, but I need to address a concern first.

You don't check for null parameters!

Does it mean that all your parameters are optional???

If so, you need to check for NullReferenceException in properties like ComponentName.

Also, if they can be null, the builder pattern might look like a good solution. But it's not. Imagine, you add one new paramter. You might forget to add the method call in one of your builder calls (Debugging hell incoming) because there's no compile check for the builder pattern, whereas the constructor will give a compile error if a parameter is missing. Now, you could argue that the .Build method checks if everything was built, and if not it throws an exception. But that's pointless because it's too late to catch that IMO. (That's a lot of trash talking about the builder)

If not, well first you'd really need to check for nulls :p

Now, lots of talk and still no solutions!

My solution is to group your parameters. I don't know your domain that much, so I'll give examples that might be bad.

You got ~11 parameters.

• QualifiedMemberName qualifiedName
• Declaration parentDeclaration
• string parentScope
• string asTypeName
• bool isSelfAssigned
• bool isWithEvents
• Accessibility accessibility
• DeclarationType declarationType
• ParserRuleContext context
• Selection selection
• bool isBuiltIn = false
• string annotations = null

Is there a link between some of them? Those parameters are like chinese to me, so I can't help. See if you can regroup them in categories. So, let's say for my example that these are the groupings :

• QualifiedMemberName qualifiedName, Declaration parentDeclaration, string parentScope, string asTypeName
• bool isSelfAssigned, bool isWithEvents, Accessibility accessibility, ,DeclarationType declarationType
• ParserRuleContext context, Selection selection, bool isBuiltIn = false, string annotations = null

Now, what is common between them? (I still don't know, but let's say I do)

This common thing will represent your "holder" class. Meaning you'd now have 3 parameters in your constructor.

public Declaration(ThatFirstGroup group1, ThatSecondGroup group2, ThatSecondGroup group3)

Excuse the poor naming, but I hope you get the point.

Is this the solution of the year? No. BUT, maybe that first group have some predefine sets of values, meaning you could "hardcode" 3 factory method that would satisfy all your cases. Maybe that second group can be created using reflection or whatever? Maybe the third one is entirely optional. That gives you some flexibility on how these set of parameters are created. If the creation of the first group is centralised in a factory, it means way less work if you ever add a parameter to that group! It would make refactoring easier.

Now, mea culpa, I don't even know if this answer fits your need since I don't know if there're some possible groupings.

I think that's the simplest solution to your problem (It's also a tip I read in a refactoring book, I don't remember the name though, I read it some years ago). There's also a stackoverflow question that gives the same answer. Turns out it's also a knowned refactoring