3
\$\begingroup\$

I'm working on a method to rotate System.Enum values that have the [System.Flag] attribute set. I've tried to think of cases where there is no work to do (set to enumVar.ALL or enumVar.NONE where no rotation can occur, or when a request is made to rotate 0 positions).

The whole of it can be seen here.

For me, the biggest issues are:

  1. I haven't done much with bit shifting, and while I understand it conceptually, I have no practical experience. I was using shifting code like inEnum >> positions | inEnum << ~positions << 1, which worked great unless it had to roll back (going beyond the maximum value and moving a bit back to the beginning, or vice versa). The method used now works fine, but requires arithmetic rather than solely bit shifting. Obviously the performance hit is virtually non-existent unless this was being run an incredible number of times, but I'd like to understand if there's a way to make it work with just bit shifting.

  2. It seems kludgy, how I'm going about using the generic TEnum in the rotation code itself. (TEnum)(object)(((int)(object)inEnum << positions | (int)(object)inEnum >> (maxRotation - positions)) & allValuesMask) is quite a handful, but boxing/unboxing and casting were the only thing I could get to work with the generic. I'm looking to understand a cleaner way to do this sort of code, either a methodology I’m not familiar with, or less mangling to achieve the same result.

Entire code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

// Requires C# 7.3 or later (Roslyn 3.0 or later) for where TEnum : System.Enum
public class Program
{
    public static void Main()
    {
        CompassDirections dirs = CompassDirections.NORTH | CompassDirections.SOUTHEAST;

        #region Test functionality
        Console.WriteLine("Valid directions are: {0}\n", dirs);

        // Rotation as a standard method
        dirs = Rotate(dirs, 2);

        Console.WriteLine("Updated directions are: {0}\n", dirs);

        // Rotation as an extension method to TEnum
        dirs.Rotate(6);

        Console.WriteLine("Updated directions are: {0}\n", dirs);
        #endregion
    }

    // Standard method
    public static TEnum Rotate<TEnum>(TEnum inEnum, int positions = 1) where TEnum : System.Enum
    {
        if (positions == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate 0 positions.  No rotation will occur.");
            return inEnum;
        }

        // Currently doesn't support any underlying enum type but int
        if (Enum.GetUnderlyingType(inEnum.GetType()) != typeof(int))
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valud on integer underlying types.  The System,Enum '" + inEnum.GetType().Name + "' does not use such an underlying value.");

        if (!typeof(TEnum).GetCustomAttributes<FlagsAttribute>().Any())
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valid for enums that have the [System.Flags] attribute set. The System.Enum '" + inEnum.GetType().Name + "' does not use that attribute.");

        /* WIP for supporting other underlying types
        Type t = Enum.GetUnderlyingType(inEnum.GetType());
        List<object> values = new List<object>((object[])Enum.GetValues(inEnum.GetType()));
        var listType = typeof(List<>).MakeGenericType(t);
        */

        List<int> values = new List<int>((int[])Enum.GetValues(inEnum.GetType()));

        if (values.Any(a => a < 0))
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valid for enums that contain no negative integer values. The System.Enum '" + inEnum.GetType().Name + "' includes negative values.");

        if (((int)(object)inEnum ^ 0) == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate an enum that is set to 'NONE = 0'. No rotation will occur.");
            return inEnum;
        }

        int allValuesMask = values.Max();

        if (((int)(object)inEnum ^ allValuesMask) == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate an enum that is set to 'ALL', 'OMNI', or a similar status including all flags. No rotation will occur.");
            return inEnum;
        }

        // Remove 0/NONE and OMNI/ALL
        values.Remove(0);
        values.Remove(values.Max());

        if (positions % values.Count == 0)
        {
            Console.WriteLine("Rotate<ENum>() called with perfect roundness positions % possiblePositions == 0. No rotation will occur.");
            return inEnum;
        }

        int maxRotation = Convert.ToString(values.Max(), 2).Length;

        while (Math.Abs(positions) > maxRotation)
        {
            if (positions > 0)
                positions -= maxRotation;
            else
                positions += maxRotation;
        }

        if (positions > 0)
        {
            return (TEnum)(object)(((int)(object)inEnum << positions | (int)(object)inEnum >> (maxRotation - positions)) & allValuesMask);
        }
        else if (positions < 0)
        {
            positions = Math.Abs(positions);
            return (TEnum)(object)(((int)(object)inEnum >> positions | (int)(object)inEnum << (maxRotation - positions)) & allValuesMask);
        }

        Console.WriteLine("Rotate<TEnum>() Unable to rotate - unknown condition.");
        return inEnum;
    }
}

public static class EnumExtensions
{
    // Extension of TEnum
    public static void Rotate<TEnum>(this ref TEnum inEnum, int positions = 1) where TEnum : struct, System.Enum
    {
        if (positions == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate 0 positions.  No rotation will occur.");
            return;
        }

        // Currently doesn't support any underlying enum type but int
        if (Enum.GetUnderlyingType(inEnum.GetType()) != typeof(int))
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valud on integer underlying types.  The System,Enum '" + inEnum.GetType().Name + "' does not use such an underlying value.");

        if (!typeof(TEnum).GetCustomAttributes<FlagsAttribute>().Any())
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valid for enums that have the [System.Flags] attribute set. The System.Enum '" + inEnum.GetType().Name + "' does not use that attribute.");

        List<int> values = new List<int>((int[])Enum.GetValues(inEnum.GetType()));

        if (values.Any(a => a < 0))
            throw new NotSupportedException("This method, Rotate<TEnum>(), is only valid for enums that contain no negative integer values. The System.Enum '" + inEnum.GetType().Name + "' includes negative values.");

        if (((int)(object)inEnum ^ 0) == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate an enum that is set to 'NONE = 0'. No rotation will occur.");
            return;
        }

        int allValuesMask = values.Max();

        if (((int)(object)inEnum ^ allValuesMask) == 0)
        {
            Console.WriteLine("Rotate<TEnum>() called to rotate an enum that is set to 'ALL', 'OMNI', or a similar status including all flags. No rotation will occur.");
            return;
        }

        // Remove 0/NONE and OMNI/ALL
        values.Remove(0);
        values.Remove(values.Max());

        if (positions % values.Count == 0)
        {
            Console.WriteLine("Rotate<ENum>() called with perfect roundness positions % possiblePositions == 0. No rotation will occur.");
            return;
        }

        int maxRotation = Convert.ToString(values.Max(), 2).Length;

        while (Math.Abs(positions) > maxRotation)
        {
            if (positions > 0)
                positions -= maxRotation;
            else
                positions += maxRotation;
        }

        if (positions > 0)
        {
            inEnum = (TEnum)(object)(((int)(object)inEnum << positions | (int)(object)inEnum >> (maxRotation - positions)) & allValuesMask);
            return;
        }
        else if (positions < 0)
        {
            positions = Math.Abs(positions);
            inEnum = (TEnum)(object)(((int)(object)inEnum >> positions | (int)(object)inEnum << (maxRotation - positions)) & allValuesMask);
            return;
        }

        Console.WriteLine("Rotate<TEnum>() Unable to rotate - unknown condition.");
        return;
    }
}

[System.Flags]
public enum CompassDirections
{
    NONE        =       0,
    NORTH       =       1 << 0,
    NORTHEAST   =       1 << 1,
    EAST        =       1 << 2,
    SOUTHEAST   =       1 << 3,
    SOUTH       =       1 << 4,
    SOUTHWEST   =       1 << 5,
    WEST        =       1 << 6,
    NORTHWEST   =       1 << 7,
    OMNI        =       ~(~0 << 8)
}

Output:

Valid directions are: NORTH, SOUTHEAST

Updated directions are: EAST, SOUTHWEST

Updated directions are: NORTH, SOUTHEAST
\$\endgroup\$
  • 4
    \$\begingroup\$ Could you add example code that explains how the method works? \$\endgroup\$ – JanDotNet Nov 9 '19 at 14:03
  • \$\begingroup\$ @JanDotNet - the fiddle has a simple use case I've used for testing. I didn't want to push a bunch of additional code into the post, but if that's the norm I certainly can. Didn't want folk to see the Wall of Code and ignore it. \$\endgroup\$ – Jesse Williams Nov 9 '19 at 14:30
  • 2
    \$\begingroup\$ It would be best to include the entire class, and any unit test code for the class. \$\endgroup\$ – pacmaninbw Nov 9 '19 at 14:33
  • 1
    \$\begingroup\$ Ok, I've added the entire class, enum, and simple use-case. \$\endgroup\$ – Jesse Williams Nov 9 '19 at 14:38
1
\$\begingroup\$
int allValuesMask = values.Max();

You probably shouldn't assume that the enum has an explicit flag set for "all of the above". It's actually somewhat counter-intuitive to do that, since the enum are flags. If you want to collect all possible bit-flags, you could instead aggregate over all values in the enum:

int allValuesMask = values.Aggregate((element, aggregate) => element | aggregate);

The same goes for this bit of code:

    // Remove 0/NONE and OMNI/ALL
    values.Remove(0);
    values.Remove(values.Max());

There's no explicit (documented) guarantee that values.Max() will set all flags, so removing it is dangerous.


(int)(object)inEnum

Boxing structs to and from objects is relatively expensive. Since you're doing this multiple times in your code, consider making a local variable.


int maxRotation = Convert.ToString(values.Max(), 2).Length;

This is kind of a hacky roundabout way of figuring out the amount of bits in values.Max(). If you're adamant on doing it like this, add a comment on what the line is supposed to accomplish.

Other options of figuring this out is calculating \$\lceil \log_2x\rceil\$:

int maxRotation = (int)Math.Ceiling(Math.Log(values.Max()) / Math.Log(2));

Or repeated right-shifting and comparing against 0. Any of these solutions should be in its own method, with proper name to document its purpose:

private static int NumberOfBits(int number) {
  if(number < 0) {
    throw new ArgumentOutOfRangeException();
  }

  var counter = 0;
  while(number != 0) {
    counter++;
    number >>= 1;
  }
  return counter;
}

Or

private static int NumberOfBits(int number) {
  if(number < 0) {
    throw new ArgumentOutOfRangeException();
  }
  return (int)Math.Ceiling(Math.Log(number) / Math.Log(2));
}

    while (Math.Abs(positions) > maxRotation)
    {
        if (positions > 0)
            positions -= maxRotation;
        else
            positions += maxRotation;
    }

This can be replaced with:

positions %= maxRotation;

Is there a difference between the two implementations you provided? Those should probably be one (private) method with two public overloads.


Is there a specific reason you implemented the extension method as taking in a ref argument? It's counterintuitive, since we try to keep structs as immutable as possible. Even worse is that while generally in method calls the ref addition makes it really explicit that a parameter is called as ref, that's not visible here:

Rotate(ref compass, 5); // We can see that compass is added as ref, so we can expect it to be mutated.
compass.Rotate(5); // We can't see it here.

I know of no system library extension methods that modify their this argument in place. Look at Linq for example. Each of these returns their result. It might be beneficial to adhere to this expected behaviour.

\$\endgroup\$
  • \$\begingroup\$ Ah, a lot of good notes here - thank you @JAD. Especially, as simple as it is, the assumption that there'd be an ALL. Thank you - will definitely work through these a bit. As for the two implementations, it was just an exercise to see how they both worked. Chances are that the extension method will not survive, but it was something worth exploring. \$\endgroup\$ – Jesse Williams Nov 12 '19 at 16:01
  • 1
    \$\begingroup\$ @JesseWilliams the idea of an extension method is fine IMO, but don't make it ref :) \$\endgroup\$ – JAD Nov 12 '19 at 16:15
5
\$\begingroup\$

First of all, I like it that you thought about all edge cases that may occur and handle that cases!

However, the result is a complicated method where lots of inputs can not be handled (which must be known by the user of the method - IMHO it is a kind of Liskov violation). At that point, you might ask yourself if an enum is the right data structure for representing your business use case.

I know it's not a code review but maybe helpful anyway ;).

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you - my day job is in software QA, so edge cases are just how I think through problems lol. I get that many inputs cannot be handled, though NotSupportedExceptions exist for that reason, right? I plan to flesh it out so that other enum types will be valid, but for my current use case it works. Enum is ideal for me for two reasons: No overhead of an additional class to provide and work with the data, just a method; and readability in the code that utilizes the enums. I considered a custom class that basically contained only valid types and supporting methods, but it was unwieldy. :) \$\endgroup\$ – Jesse Williams Nov 9 '19 at 14:35
  • \$\begingroup\$ Just a note, given strictly the compass directions as an example here, it also allows variation simply by updating the enum. I could get rid of ordinal directions and it would still work. Or I could add subordinal directions (NNE, ENE, ESE, SSE, SSW, WSW, WNW, NNW) and it would function as well. \$\endgroup\$ – Jesse Williams Nov 9 '19 at 14:44
1
\$\begingroup\$

Complexity

The function Rotate() is too complex (does too much). There is a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Another reason to keep functions simple is that they are easier to write, debug, read, and maintain. As a suggestion, all the error checking can be moved into another function that Rotate calls, leave only the rotation portion in the Rotate function itself.

Be Consistent with the if Statements

This is partially about style, but also about maintainability. The if statements that contain throw statements do not have blocks of code, but most of the other if statements do. As time goes on code needs to be maintained, many times a bug fix may be a simple insertion of a new statement, it is easier to maintain the code if the if and else statements have blocks of code even if only a single statement is within the block. This is also true for loops.

Debug Code and Unused Code

The code contains this comment block:

        /* WIP for supporting other underlying types
        Type t = Enum.GetUnderlyingType(inEnum.GetType());
        List<object> values = new List<object>((object[])Enum.GetValues(inEnum.GetType()));
        var listType = typeof(List<>).MakeGenericType(t);
        */

When posting code for review, it is better remove unused code and put debug code into

#if DEBUG
...
#endif // DEBUG

This makes it easier to review the code. Features that haven't been implemented are not meant for code reviews. Removing unused code makes the remaining code easier to read, debug and modify.

\$\endgroup\$
  • \$\begingroup\$ I’m not really sure how this violates SRP, given that the only function it performs is rotation. \$\endgroup\$ – Jesse Williams Nov 9 '19 at 16:20
  • \$\begingroup\$ @JesseWilliams It is performing error checking as well as rotation, as I said move the error checking into it's own function, call that function from rotate. The error checking may be valid in other functions if features are added to the program. Generally any function that doesn't fit in a screen is way too complex and probably doesn't follow SRP. In this case there is a clear violation of SRP. \$\endgroup\$ – pacmaninbw Nov 9 '19 at 16:24
  • \$\begingroup\$ Ok - thanks. Any thoughts regarding the two questions I had? \$\endgroup\$ – Jesse Williams Nov 9 '19 at 16:36
  • \$\begingroup\$ @JesseWilliams I might have used the enumType with dictionaries to achieve the same result. \$\endgroup\$ – pacmaninbw Nov 9 '19 at 17:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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