2
\$\begingroup\$

I've written a method that is going to set a bit array by passing bool parameters to be able send correct command

private byte _mode;

     public void SetConfiguration(
            bool mode1,
            bool mode2,
            bool mode3,
            bool mode4,
            bool mode5,
            bool mode6,
            bool mode7,
            bool mode8)
        {
            var setBits = new BitArray(8);

            setBits.Set(0, mode1);
            setBits.Set(1, mode2);
            setBits.Set(2, mode3);
            setBits.Set(3, mode4);
            setBits.Set(4, mode5);
            setBits.Set(5, mode6);
            setBits.Set(6, mode7);
            setBits.Set(7, mode8);

            byte[] byteSet = new byte[1];
            setBits.CopyTo(byteSet, 0);
            _mode = byteSet[0];
        }

So depending on what command the application is sending I am going to set it with boolean. What bothers me is that the method has too many bool parameters and I'm wondering if anyone have an idea how to write this in a better way

\$\endgroup\$
1
  • \$\begingroup\$ Do you know that there is a BitArray ctor which accepts bool[]? \$\endgroup\$ May 6, 2021 at 7:21

3 Answers 3

1
\$\begingroup\$

You could use a params array to pass the parameters

public void SetConfiguration(params bool[] modes)
{
    var setBits = new BitArray(8);

    for (int i = 0; i < modes.Length; i++)
    {
        setBits.Set(i, modes[i]);
    }

    byte[] byteSet = new byte[1];
    setBits.CopyTo(byteSet, 0);
    _mode = byteSet[0];
}

You might want to add tests to ensure modes is not longer than 8 or you could expand the code to automatically set more than one byte in such cases. Shorter should be okay.

You can call it with individual parameters or with a bool array.

SetConfiguration(false, true, true, false, true, true, true, false);

var modes = new bool[] {false, true, true, false, true, true, true, false};
SetConfiguration(modes);

\$\endgroup\$
2
  • \$\begingroup\$ What if modes.Length were different from 8? \$\endgroup\$
    – aepot
    May 5, 2021 at 15:30
  • 1
    \$\begingroup\$ This is why I suggested to add a test. \$\endgroup\$ May 5, 2021 at 15:36
0
\$\begingroup\$

I am going to expand upon @OlivierJacot-Descombes answer and offer an alternative. You do not have to limit yourself to a single method signature. And you do not have to use a BitArray. My answer is strictly offered as an alternative but I make no claim as to performance. But since the backing data storage to a BitArray is an internal int[], much of the bit-twiddling I perform is also performed by BitArray.

public void SetConfiguration(bool mode1,
                             bool mode2,
                             bool mode3,
                             bool mode4,
                             bool mode5,
                             bool mode6,
                             bool mode7,
                             bool mode8) 
                             => SetConfiguration(new bool[] { mode1, mode2, mode3, mode4, mode5, mode6, mode7, mode8 });

public static void SetConfiguration(params bool[] modes)
{
    if (modes == null)
    {
        throw new ArgumentNullException(nameof(modes));
    }
    if (modes.Length != 8)
    {
        throw new ArgumentException($"Array {nameof(modes)} must have a Length of 8.");
    }

    byte result = (byte)(modes[0] ? 1 : 0);

    for (int i = 1; i < modes.Length; i++)
    {
        if (modes[i])
        {
            result += (byte)(2 << (i - 1));
        }

    }

    _mode = result;
}

MORE THINGS TO CONSIDER

ONE BIT AT A TIME If you intend to sometimes fiddle with only a single flag or bit at a time, you may want to consider this method signature:

public void SetConfiguration(bool mode1 = false,
                             bool mode2 = false,
                             bool mode3 = false,
                             bool mode4 = false,
                             bool mode5 = false,
                             bool mode6 = false,
                             bool mode7 = false,
                             bool mode8 = false) 
                             => SetConfiguration(new bool[] { mode1, mode2, mode3, mode4, mode5, mode6, mode7, mode8 });

NAMING Your name choice of mode reflects the mode of configuration. I get that. But these methods are manipulating flags or bits, so the naming could be changed to reflect that with parameters of flag1 or bit1. BUT I would also then suggest the parameter names should reflect 0-indexing. Thus, flag0 or bit0.

RETURN INT The SetConfiguration is a void but it sets a class level object named _mode. The bit-twiddling part could be a more generalized, static method that returns an int. This would make it reusable anywhere you want to twiddle with bits and return an integer. If you do this, then the method name would need to be changed since it no longer sets a value, but rather would get a value.

\$\endgroup\$
0
\$\begingroup\$

Consider making the settings more readable and strong-typed:

public enum Modes{
    Mode1 = 1,
    Mode2 = 2,
    Mode3 = 4,
    Mode4 = 8,
}

public class Config
{
    private Modes configVal;
    
    public void SetConfig(Modes modes){
        this.configVal = modes;
    }
    
    public bool AreAllSet(Modes modesAllSet)
    {
        return ((int)this.configVal & (int)modesAllSet) == (int)modesAllSet;
    }

    public bool AreAnySet(Modes modesAllSet)
    {
        return ((int)this.configVal & (int)modesAllSet) != 0;
    }

}

Then you can use them like this:

    var conf = new Config();
    conf.SetConfig(Modes.Mode1 | Modes.Mode2);

    Console.WriteLine("Mode 1: " + conf.AreAllSet(Modes.Mode1));
    Console.WriteLine("Mode 2: " + conf.AreAllSet(Modes.Mode2));
    Console.WriteLine("Mode 1+2: " + conf.AreAllSet(Modes.Mode1 | Modes.Mode2));

    Console.WriteLine("Mode 3: " + conf.AreAllSet(Modes.Mode3));
    Console.WriteLine("Mode 1+3: " + conf.AreAllSet(Modes.Mode1 | Modes.Mode3));

    Console.WriteLine("Mode 1|3: " + conf.AreAnySet(Modes.Mode1 | Modes.Mode3));
  • You still have the bit representation in the configVal field if it's needed
  • You can choose which bit to set in a single parameter, no need to pass 8 of them to set one
  • You could implement Set and Unset methods that modify the settings instead of replacing them: configVal |= modes to set or configVal = (configVal | modes) ^ modes to unset
  • you could still implement bit array or bool methods for init, but your code for using the settings will be cleaner
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.