TL;DR Scroll to the very bottom.
From what I see, you are implementing a new language on top of C#. The enums seems to denote the type of an identifier (or, anything that has a name and that appears in the new language's source code), which seems to be applied to nodes that are to be added into a tree representation of the program.
In this particular situation, there are very few polymorphic behaviors between the different types of nodes. In other words, while it is necessary for the tree to be able to contain nodes of very different types (variants), the actual visitation of these nodes will basically resort to a giant if-then-else chain (or instanceof / is checks). These giant checks will likely happen in many different places across the project. This is the reason why enums may seem helpful, or, they're at least as helpful as instanceof / is checks.
Visitor pattern might still be useful. In other words, there are various coding styles that can be used instead of the giant chain of instanceof. However, if you want a discussion on the various benefits and drawbacks, you would have chosen to showcase a code example from the ugliest chain of instanceof in the project, instead of quibbling about enums.
This is not to say classes and inheritance hierarchy aren't useful. Quite the opposite. While there aren't any polymorphic behaviors that work across every declaration type (aside from the fact that every declaration must have a Name property), there are plenty of rich polymorphic behaviors shared by nearby siblings. For example, Function and Procedure probably share some behaviors (both being callable and accepting a list of typed input arguments), and PropertyGet will definitely inherit behaviors from Function (both having a ReturnType). You might use either enums or inheritance checks for the giant if-then-else chain, but the polymorphic behaviors, however fragmented, must still be implemented in classes.
There're many online advice against overuse of instanceof / is checks. Performance is not one of the reason. Rather, the reason is to prevent the programmer from organically discovering suitable polymorphic behaviors, as if instanceof / is is a crutch. But in your situation, you have no other choice, since these nodes have very little in common.
Now here are some concrete suggestions.
There are several ways of representing the non-leaf groupings.
Compare the following excerpt of your original code...
[Flags]
public enum DeclarationType
{
Member = 1 << 7,
Procedure = 1 << 8 | Member,
Function = 1 << 9 | Member,
Property = 1 << 10 | Member,
PropertyGet = 1 << 11 | Property | Function,
PropertyLet = 1 << 12 | Property | Procedure,
PropertySet = 1 << 13 | Property | Procedure,
LibraryFunction = 1 << 23 | Function,
LibraryProcedure = 1 << 24 | Procedure,
}
to this modified version:
[Flags]
public enum DeclarationType
{
Nothing = 0, // to facilitate bit testing
// Let's assume Member is not a concrete thing,
// which means it doesn't need its own bit
/* Member = 1 << 7, */
// Procedure and Function are concrete things; meanwhile
// they can still have sub-types.
Procedure = 1 << 8,
Function = 1 << 9,
Property = 1 << 10,
PropertyGet = 1 << 11,
PropertyLet = 1 << 12,
PropertySet = 1 << 13,
LibraryFunction = 1 << 23,
LibraryProcedure = 1 << 24,
// new
Procedures = Procedure | PropertyLet | PropertySet | LibraryProcedure,
Functions = Function | PropertyGet | LibraryFunction,
Properties = PropertyGet | PropertyLet | PropertySet,
Members = Procedures | Functions | Properties,
LibraryMembers = LibraryFunction | LibraryProcedure
}
This modified version avoids allocating bits toward non-concrete declaration types. Instead, non-concrete declaration types (abstract groupings of declaration types) simply have enum values which are the bitwise-or (union of the bits) across all of its children.
There is a caveat: if there is an abstract declaration type that has a single child, and if there's a need to distinguish between the abstract one (parent) from the concrete one (child), then the abstract one will still need its own bit.
One caveat that is specific to this question: a Property is initially an identifier (when you just see its name, without seeing how it is used in the code), but it may transmute into PropertyGet / PropertyLet / PropertySet as soon as you see how it is being used in the code. In other words, at different stages of parsing, you might either need to mark a Property identifier as being "this name refers to a property", and later change that to "this line of code is accessing this property in a certain way".
To resolve this caveat, you might need two sets of enums; one enum denotes what a name (identifier) is; another enum denotes what the code is trying to do (e.g. declaring the body of something; trying to use something in a certain way).
Consider whether the auxiliary information about each enum value can be read off from an array instead.
This suggestion is mutually exclusive with other suggestions because it requires converting powers-of-two values back to small non-negative integer values.
public enum DeclarationType
{
Procedure = 8,
Function = 9,
Property = 10,
PropertyGet = 11,
PropertyLet = 12,
PropertySet = 13,
LibraryFunction = 23,
LibraryProcedure = 24,
}
static readonly bool[] DeclarationTypeIsMember = new bool[32]
{
?, ?, ?, ?, ?, ?, ?, ?, // bit[0] ... bit[7]
true, true, true, true, true, true, ?, ?, // bit[8] ... bit[15]
?, ?, ?, ?, ?, ?, ?, true, // bit[16] ... bit[23]
true, ... // bit[24] ...
}
static bool IsMember(DeclarationType dt)
{
int intValue = (int)dt;
return (intValue < 0 || intValue >= 32) ? false : DeclarationTypeIsMember[intValue];
// you can also throw an exception if the enum is outside range.
}
// likewise for IsFunction(dt), IsProcedure(dt), IsProperty(dt), ...
Maintainability is going to be problematic.
Check whether a one-to-one mapping between C# types (classes in an inheritance hierarchy) and your enum values.
(Alternatively, you can tweak your enum values to ensure a one-to-one mapping with types.)
In C#, a lot of libraries abuse the nifty Type object.GetType() method, for good or bad.
Anywhere you're storing the enum as a value, you might ask yourself whether you can store the Type as a value instead.
To use this trick, you can initialize two read-only hash tables, namely:
// For disambiguation, I'll assume that the actual
// (behavior-implementing) classes are under the
// "Lang" namespace.
static readonly Dictionary<Type, DeclarationType> TypeToDeclEnum = ...
{
{ typeof(Lang.Procedure), DeclarationType.Procedure },
{ typeof(Lang.Function), DeclarationType.Function },
{ typeof(Lang.Property), DeclarationType.Property },
...
};
static readonly Dictionary<DeclarationType, Type> DeclEnumToType = ...
{
// same as the first dictionary;
// just swap the key and the value
...
};
The final vindication for those suggesting classes and inheritance hierarchy...
Once you can see that the enums are an approximation to the inheritance hierarchy, the following advice holds:
- Design (or improve) your inheritance hierarchy first,
- Then go back and design your enums to approximate that inheritance hierarchy.
DeclarationType. If I want to determine whether or notxis a subtype ofy, I'm probably going to want to write that asx.IsSubtypeOf(y), not asx && y == y.x.HasFlag(DeclarationType.Member)does....HasFlaginstead ofIsSubtypeOf, then I need some other way of finding out that what it really means is "is subtype of". You could create an extension method, but as a user, what I would find least surprising is forDeclarationTypeto just be a struct that hasIsSubtypeOfas a real method.