4
\$\begingroup\$

I want to set flags which are given in string representation in any order. And it is intimidating, because you have to check like 2^3 possible options in my occassion. I was just wondering is there any better way to do that than just simple brute-force. Any optimization and readability improvment would be great. This is my code:

[Flags]
public enum salaryFeatures
{
Children = 1,
Graduate = 2,
Disability = 4
}

class Program
{

public static void Main()
{
    string Employee = "10023 Mark Male 6.7 70 30 20 Children Graduate";
    salaryFeatures f;
    f = GetFeaturesByString(Employee);
}

public static salaryFeatures GetFeaturesByString(string fields)
{
    bool children = false;
    bool graduate = false;
    bool disability = false;
    salaryFeatures features = new salaryFeatures();

    if (fields.ToLower().Contains("children")) children = true;
    if (fields.ToLower().Contains("graduate")) graduate = true;
    if (fields.ToLower().Contains("disability")) disability = true;

    if (children && graduate && disability)
    {
        return features = salaryFeatures.Children | salaryFeatures.Disability | salaryFeatures.Graduate;
    }
    if (children && graduate)
    {
        return features = salaryFeatures.Children | salaryFeatures.Graduate;
    }
    if (children && disability)
    {
        return features = salaryFeatures.Children | salaryFeatures.Disability;
    }
    if (children)
    {
        return features = salaryFeatures.Children;
    }
    if (graduate && disability)
    {
        return features = salaryFeatures.Graduate | salaryFeatures.Disability;
    }
    if (graduate)
    {
        return features = salaryFeatures.Graduate;
    }
    if (disability)
    {
        return features = salaryFeatures.Disability;
    }
    return features;
}
}
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Here is my take on your code

[Flags]
public enum SalaryFeatures
{
  None= 0,
  Children = 1,
  Graduate = 2,
  Disability = 4
}

class Program
{

  public static void Main()
  {
    string Employee = "10023 Mark Male 6.7 70 30 20 Children Graduate";
    SalaryFeatures f;
    f = GetFeaturesByString(Employee);
    // f.Dump(); //LinqPad only
  }

  public static SalaryFeatures GetFeaturesByString(string fields)
  {
    SalaryFeatures features = SalaryFeatures.None;

    if (fields.IndexOf("children", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Children;
    if (fields.IndexOf("graduate", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Graduate;
    if (fields.IndexOf("disability", StringComparison.OrdinalIgnoreCase) >= 0) features |= SalaryFeatures.Disability;

    return features;
  }
}

First of the definition of your Enumeration is not complete. You need to define a None value for it

Use None as the name of the flag enumerated constant whose value is zero. You cannot use the None enumerated constant in a bitwise AND operation to test for a flag because the result is always zero. However, you can perform a logical, not a bitwise, comparison between the numeric value and the None enumerated constant to determine whether any bits in the numeric value are set.

See also the Guidelines when defining a Flags enum. I also change the naming of the enum to PascalCase (SalaryFeatures).

After that I have improved your method GetFeaturesByString to use bitwise OR to combine the values depending on the values that are found in your string. I also use the method IndexOf(string, StringComparison) to check if the search string exists in the source string, instead of using ToLower() and Contains().

\$\endgroup\$
2
  • \$\begingroup\$ Can you say why you use StringComparison instead of contains()? Is it faster or just more appropriate somehow? \$\endgroup\$
    – Unknown
    Oct 20, 2016 at 8:57
  • 1
    \$\begingroup\$ @Unknown I want to avoid the call to ToLower() cause you are only interested if the string is present (ignoring casing) in the source string. So I choose IndexOf with StringComparison.OrdinalIgnoreCase. This returns a value greater than or equal 0 if the string was found. \$\endgroup\$
    – Jehof
    Oct 20, 2016 at 9:02
3
\$\begingroup\$

It would be a good idea to extract the salary features from the string before you start processing them. You should put this in method (extension?) that specializes in this.

var text = "10023 Mark Male 6.7 70 30 20 Children Graduate";
var fieldNames = Enum.GetNames(typeof(salaryFeatures)).Where(n => text.IndexOf(n) >= 0);

Now after having found the fields you can parse them in another method, ignore the case and build the flags with linq's Aggregate extension:

var salaryFeatures = fieldNames.Aggregate(SalaryFeatures.None, (result, next) 
    => result |= (SalaryFeatures)Enum.Parse(typeof(SalaryFeatures), next, true));
  • use the Aggregate extension to concatenate the flags
  • use the ignoreCase = true
  • the salaryFeatures should actually be named with PascalCase SalaryFeatures
  • if you defined one additional flag None you could use it instead of (salaryFeatures)0

Both these changes will allow you to add more flags in future without modifying those methods.

\$\endgroup\$
4
  • \$\begingroup\$ This looks reasonable, since bit confusing. I have to take some time to understand this completely, anyway, thank you! \$\endgroup\$
    – Unknown
    Oct 20, 2016 at 8:43
  • 2
    \$\begingroup\$ Enum.Parse() can understand a comma separated string e.g Enum.Parse(typeof(SalaryFeatures), string.Join(",", fieldNames), true) \$\endgroup\$
    – AlanT
    Oct 20, 2016 at 9:18
  • \$\begingroup\$ @AlanT cool, I didn't know that. Then there is no need for the Aggregate. \$\endgroup\$
    – t3chb0t
    Oct 20, 2016 at 9:22
  • \$\begingroup\$ In my experience attempts to map string values to Enum using Parse method eventually backfire due to various reasons: localization, changes in format/protocol, etc. So I tend to just manually map the values straight away. I feel like the only situation where parsing is the best option, is if those strings are actually produced by SalaryFeatures.ToString() call in the first place. \$\endgroup\$
    – Nikita B
    Oct 20, 2016 at 15:09
1
\$\begingroup\$

Here is another variant using the Aggregate extension method:

https://dotnetfiddle.net/7RrGTE

[Flags]
public enum SalaryFeatures
{
    None = 0,
    Children = 1,
    Graduate = 2,
    Disability = 4
}

//...

    var employee = "Mark: Disability Male with Children"; 
    var features =
        Enumerable.
        Range(0, Enum.GetValues(typeof(SalaryFeatures)).Length - 1).
        Aggregate
        (
            SalaryFeatures.None,
            (f, b) =>
                employee.
                Contains(
                ((SalaryFeatures)(1 << b)).
                ToString()) ?
                f | (SalaryFeatures)(1 << b)
                :
                f
        );
    var f = (int)features;
    Console.WriteLine(f); // expected: 5

(where "Length - 1" is to account for the presence of SalaryFeatures.None)

\$\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.