Explicitly defining flag combinations in an enum

Question

I was thinking of implementing an enum that defines the state of a game object, and I wanted to know if I could directly use flags within the enum's definition, instead of defining the object's state as a collection of flags with no easy, pre-defined, global name for the states used in the state machine.

For example, let's say there are 5 states: PreActivation (Created but not started; i.e. an enemy in a future wave), Active (Currently in use; i.e. an enemy on the screen, attacking you), Paused (No longer active, but may reactivate; i.e. an enemy if the player uses a time-freezing power), DeActivated (An object whose finished use but is still in the game world; i.e. an enemy whose body is left after death like in Doom 1 & 2), and ToRemove (An object slated for removal from the game; i.e. an enemy after you clear a level and move to the next one).

What I want to do is define the enum so the states hold all applicable flags; for instance, a DeActivated enemy: 1. Has been previously activated, and 2. Isn't currently active. My current thinking is doing something like this:

public enum ObjectState
{
    // The first section are the flags
    BeenActivated   = 0b0000001, // Previously activated
    CurrentlyActive = 0b0000010, // Currently activated
    IsSuspended     = 0b0000100, // It may be reactivated
    ShouldRemove    = 0b0001000, // It should be removed
    // These are the states
    PreActivation   = 0b0000100, // Mot currently active, nor has it ever been active, but it will get activated
    Active          = 0b0000011, // Currently active,     and it's been active
    Paused          = 0b0000101, // Not currently active, but it's been active before
    DeActivated     = 0b0000001, // Not currently active, but it's been active before, and it shouldn't get reactivated, but don't remove yet
    ToRemove        = 0b0001001  // Not currently active, but it's been active before, and it shouldn't get reactivated, it should be removed
}

As far as I know, this should work correctly, but I have a few main concerns:

1. Are there any problems likely to come from this?
2. Is this bad practice?
3. Is this bad practice? And, if it is;
   • A. What's wrong with it?
   • B. What should I do instead? I'd just make the object's state a collection of these flags, but I'd like a shorthand enum for specific states, as this allows for complexity for specific instances and simplicity when it's needed. Is there a more acceptable way to achieve this?

Sorry if this is a repeat or I broke some other rule, but I just created an account today; this is my 1st post. Plus, I'm not sure what you would call this when searching, and I didn't get any similar hits from here or Google.

Esau

Just add the [Flags] attribute to the enum and you're done, no problem at all. Also, you can use (1 << 0), (1 << 1), (1 << 2) and so on for the values if you don't want to have binary numbers (just a matter or preference). – Sphygmo

I was thinking about using the << operator, but I find it easier to understand if I see where the flags are. Plus it would require a bit more work for the last few values. If you're suggesting that the Flag attribute would fix the problem, I think you missed the idea. It's not about just setting up an enum to use flags; it's about using flags inside an enum with other values, whether this is good practice or not, and if there is a more acceptable use for it. If I added the attribute, the problem might actually be aggravated; I want to use this enum as both a normal enum and a flag-using enum. – Esau

This looks incredibly messy, and I definitely think this will be bad practice. Why mix flags and states in the same type? It will likely be quite unreadable and hard to maintain the related source. – Koloski

Of course for the last values is better to use anything else than bit shifting, but again, that was a comment about something I prefer for code reading. Also, having mixed flags and values is only a matter of preference, ones will tell you is a bad practice, others that's a common usage (per state machines per example), so it's not very well fitted for S.O. – Sphygmo

I've just never seen this done and couldn't find it anywhere, so I wasn't sure if this was universally considered bad practice or if it was just uncommon. – Esau

Answer 1

You can do so. It's the very point of flag enums. If an enum is intended to work as flags, mark it with the [Flags] attribute.

I would suggest to combine the existing flags with a bitwise or (|) instead. It's more readable and less error-prone.

[Flags]
public enum ObjectState
{
    // Flags
    BeenActivated   = 0b0000001, // Previously activated
    CurrentlyActive = 0b0000010, // Currently activated
    IsSuspended     = 0b0000100, // It may be reactivated
    ShouldRemove    = 0b0001000, // It should be removed

    // States as combination of flags.
    PreActivationState   = IsSuspended,                     // Mot currently active, nor has it ever been active, but it will get activated
    ActiveState          = BeenActivated | CurrentlyActive, // Currently active,     and it's been active
    PausedState          = BeenActivated | IsSuspended,     // Not currently active, but it's been active before
    DeActivatedState     = BeenActivated,                   // Not currently active, but it's been active before, and it shouldn't get reactivated, but don't remove yet
    ToRemoveState        = BeenActivated | ShouldRemove     // Not currently active, but it's been active before, and it shouldn't get reactivated, it should be removed
}

I also added a "State" suffix to the states to better differentiate them from flags. Or turn it around and add a "Flags" suffix to the flags instead.

Civility

That Pause definition was a typo on my part, but this was exactly what I was hoping for. And that's definitely a better way to write both the names and the definitions. – Esau

And, not only is it more readable, but if you changed BeenActivated to 0b00000010 for whatever reason, you don't need to change it again in all the other areas – Unmeet

Downvoted because while it is doable, in OPs case it is bad practice, as it is used to govern state in an OO design , by conditional logic. – Test

Answer 2

Did you consider using a [Flags]-enum?

[Flags]
public enum ObjectState
{
    BeenActivated   = 1, // Previously activated
    CurrentlyActive = 2, // Currently activated
    IsSuspended     = 4, // It may be reactivated
    ShouldRemove    = 8, // It should be removed
}

// Currently active, and it's been active
var active = ObjectState.BeenActivated | ObjectState.CurrentlyActive;

Unclasp

That's the idea of this; it's basically a flag enum with pre-defined states. For example, instead of checking if it's BeenActivated, is not CurrentlyActive, and IsSuspended, I'd just check if ObjectState state is paused with state == ObjectState.Paused. It's faster, more easily readable, and better to quickly swap values. – Esau

Yep, I understand your initial intention. This is just a hint, that there's a built-in language feature which may suit your needs. 👍 – Drub

Answer 3

It is hard to know really without more context, but you may wish to look into State Pattern for a more extensible and OO way of doing things.

The way it works is that you make your game object an empty shell, implementing the same interface an abstract Game State class does. The game object will call the state to do what the interface specifies, while the state has a reference to its containing game object. When state changes, the state instance tells the game object.

So for example, let us say your game object has an interface with one method only, "Attack". The abstract state class has two concrete subclasses, Alive and Dead. Alive and Dead both implement Attack too. When something attacks your game object, object.Attack(); , the object internally just calls state.Attack(). The Alive state will decide if the attack was successful and do parent.State=new DeadState();. When attack is called again, DeadState does nothing.

This way you avoid enums, switches, ifs,everything, and you just add gamestates without further programming. Your "been activated" check would be an interface method too.

Test

I'll take another look into that, but from what I've seen, I think my usual method of FSM creation is more effective for me. I generally have an enum that lists a few simple states; based on what it is, the methods will behave slightly (but not wholly) differently. In addition, object managers and other objects can check the state and make decisions from it (i.e. I used the ToRemove state in a Bullet class so the BulletManager knew to remove it from the tree). Besides, I want to know if using enums like this is considered good practice, as I plan on using this in other applications if it is. – Esau

Yes, switches and if-else-chains are often a sign of a lack of OO. – Civility

@JMor That's the point. Using enums to represent state in an OO design is bad practice. BulletManager is a bad idea. Just have the environment call Remove on the bullet. The Bullet State decides for you. Using flags in enums is a trivially simple language feature. Bad or good practice depends on context, and in this context it sounds like abuse rather than use. To make similar functions share code, place common code in the abstract base state class. – Test

@Test I'm probably in the wrong here (I'm referencing my 1st project), but I'm not sure why having a BulletManager is bad; it's a quad tree containing Bullets, and Bullets are changed because the manager checks for collision. I can't remove a Bullet from inside itself (as far as I know), and the Bullets exist only in the manager, so I'd assume removing the reference from the manager is the way to do it. It's totally possible that the project's core is wrong, but I don't see how that helps. Also, which core principle of OO design does it violate? I want to do some research. – Esau

@JMor In your case, something should be responsible for detecting collisions. At first glance I would be tempted to introduce a PhysicsEngine strategy to something like World. I would then look into Rx for reactive streams of collisions, make all objects potential subscribes, and simply call Collide on all participating objects. If this results in removal, well, why not .Ricochet() etc? – Test

@JMor Use concepts like World to describe the behavior of the world, not BulletManager. Let Bullet 'react' to World. So if a collision happens, let world tell bullet what happened, and let bullet decide what happens next. If bullet needs World's help to decide if to .Ricochet or to .Remove, then expose support methods from World to Bullet. Read Design Patterns by GoF – Test

@JMor BulletManager is problematic because it is both avoiding tying behavior to the object (encapsulation) and it hugely leaks details to the outside world, (leaky abstraction). Think of the method ball.Kick(). The implementation of a football should behave when kcked depends on the ball, the football. Not on a class BallManager. – Test