tags:

views:

417

answers:

3

I have two flags:

[Flags]
enum Flags
{
  A = 1,
  B = 2
};

I set them like this:

Mode = Flags.A | Flags.B; // default value
for(int i = 0; i < args.Length; i++) {
switch(args[i])
{
  case "--a":
  {
    if ((Mode & Flags.A) == Flags.A && (Mode & Flags.B) == Flags.B) 
     // both, default assume
    {
      Mode = Flags.A; // only A
    }
    else
    {
      Mode |= Flags.A; // append A
    }
    break;
  }
  case "--b":
  {
    if ((Mode & Flags.A) == Flags.A && (Mode & Flags.B) == Mode.B)
    {
      Mode = Flags.B;
    }
    else
    {
      Mode |= Flags.B;
    }
    break;
  }
} }

and use them later like this:

if((Mode & Flags.A) == Flags.A)
{
 //
}
if((Mode & Flags.B) == Flags.B)
{
 //
}

Major reservation: Both flags may be set. Or just one, in this case only one part of code is executed.

Is my code good? What is the best way to setup flags?

Upd: Is this less ugly then first, how do you think?

Mode = 0; // default value now is empty
for(int i = 0; i < args.Length; i++) {
switch(args[i])
{
  case "--a":
  {
    Mode |= Flags.A;
    break;
  }
  case "--b":
  {
    Mode |= Flags.B;
    break;
  }
} }
if(Mode == 0)
{
  Mode = Flags.A | Flags.B; // if no parameters are given, setup both flags
}
+2  A: 

Here's how I would set my flags:

Mode = 0;
for(int i = 0; i < args.Length; i++) {
    switch(args[i]) {
    case "--a":
        Mode |= Flags.A;
        break;
    case "--b":
        Mode |= Flags.B;
        break;
    }
}

If both flags should be on by default, I think it makes more sense to change the command line parameters to something like --not-a and --not-b. This would both reflect the default setting, and let you get rid of (Mode & Flags.A) == Flags.A && (Mode & Flags.B) == Flags.B, which is rather ugly, IMHO.

Then you can set your flags like this:

Mode = Flags.A | Flags.B;
for(int i = 0; i < args.Length; i++) {
    switch(args[i]) {
    case "--not-a":
        Mode &= ~Flags.A;
        break;
    case "--not-b":
        Mode &= ~Flags.B;
        break;
    }
}

Finally, if you have a lot of flags (instead of just two), it might be easier to set up your enum like this:

[Flags]
enum Flags
{
    A = 1,
    B = 1 << 1,
    C = 1 << 2,
    D = 1 << 3,
    E = 1 << 4,
    F = 1 << 5
};
Can Berk Güder
Main remark is about default value is A | B. So just appending using |= is not enough.
abatishchev
I updated my answer.
Can Berk Güder
I agree about ugliness of that code, look please at head post, what do you think?
abatishchev
The new code is much better, but I still think --not-a and --not-b make more sense. On the other hand, Mode will never be null (you set it to 0), so you should check for Mode == 0.
Can Berk Güder
+2  A: 

You can turn a "bit" off with the following wonderful statement:

Mode &= ~Flags.A;

I'd reccoment including a "null" value in your enum as well:

[Flags] enum Flags { Null = 0; A = 1, B = 2; }

It will keep your life simpler! :-)

danbystrom
abatishchev
danbystrom
+1  A: 

The second version is much better - this is exactly what I would do. Change Mode == null to Mode == 0 though.

Mode = 0; // default value now is empty
for(int i = 0; i < args.Length; i++) {
    switch(args[i])
    {
        case "--a":
            Mode |= Flags.A;
            break;

        case "--b":
            Mode |= Flags.B;
            break;
    }
}

if(Mode == 0)
{
    Mode = Flags.A | Flags.B; // if no parameters are given, setup both flags
}
configurator