patterncsharpModerate
Declaration of bloatedness: the class that knew too much?
Viewed 0 times
thedeclarationmuchknewtoothatbloatednessclass
Problem
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
Then the
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
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
{
///
/// Defines a declared identifier.
///
[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 isBu
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
{
///
/// Defines a declared identifier.
///
[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 isBu
Solution
Implement IEquatable
Generally speaking, whenever I find myself writing overrides for
Generally speaking, whenever I find myself writing overrides for
object.Equals and object.GetHashCode, I also make the type implement IEquatable. It's generally pretty easy to do so by calling the IEquatable.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;
}Code Snippets
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;
}Context
StackExchange Code Review Q#106874, answer score: 10
Revisions (0)
No revisions yet.