0
\$\begingroup\$

From the related questions here, I've found this and this answers, both of which suggest Replacing Conditional with Polymorphism, are probably what I'm looking for. However, I'm not sure if polymorphism would be a good choice for my particular case. Below are the relevant code snippets (sorry they're incomplete).

public enum Code : byte
{
    A = 0x01,
    B,
    C,

    // there's more

    a - 0x41,
    b,
    c,
    // there's more
}

The class constructor:

Code? code = null;
byte[] data = null;

public ContentHolder(Code code, byte[] data)
{
    this.code = code;
    this.data = data;
}

The switch statement:

int length = data.Length;

switch(code)
{
    case Code.A:
    case Code.E:
    case Code.F:
    case Code.G:
        QR = data[0];
        if (code == Code.F)
            // set another property
        break;
    case Code.B:
        if (length < 13)
            throw new ArgumentException();
        Index = data[0];                    
        break;
    case Code.C:
        int content_length = data[1];
        if (length < content_length + 30)
            throw new ArgumentException();
        Flag = (ContentFlag)data[0];
        ContentLength = data[1];
        if (Flag & ContentFlag.FlagA == ContentFlag.FlagA) 
        {
            Content = new byte[content_length];
            Array.Copy(data, 2, Content, 0, content_length);
        }
        if (Flag & ContentFlag.FlagB == ContentFlag.FlagB) 
        {
            // set property
        }
        // 5 more flags
        break;
    case Code.D:
        int genre = data[2];
        int genre_length = 2;
        switch (genre) 
        {
            case Genre.TypeA:
                genre_length += 4;
                break;
            case Genre.TypeB:
                genre_length += 8;
                break;
            case Genre.TypeC:
                genre_length += 1;
                break;
            case Genre.TypeD:
                genre_length += 15;
                break;
        }
        if (length != genre_length)
            throw new ArgumentException();
        Array.Copy(data, 0, Target, 0, 2);
        switch (genre) 
        {
            case Genre.TypeA:
                GenreID = BitConverter.ToUInt32(data, 2);
                break;
            case Genre.TypeB:
                GenreID = BitConverter.ToUInt64(data, 2);
                break;
            case Genre.TypeC:
                GenreID = data[2];
                break;
            case Genre.TypeD:
                GenreID = ASCIIEncoding.ASCII.GetString(data, 2, 15);
                break;
        }
        break;
default:
    throw new ArgumentException();

Basically, I have a class that takes in a byte array and a code. Based on the code, I am setting properties of the class. There will be more than 20 cases in total, each case may or may not have precondition verifications and may or may not be handled differently, as shown in the snippets. I feel this is already getting pretty hard to read and maintain, so I might end up with 20 implementation classes if I use polymorphism. What would the best way to refactor this?

Update

I guess my main concern is that I may have too many implementation classes if I use one for each case (except for the common ones which is about 10-15% of all cases), not to mention there'll likely be more codes added with other ways of being handled, and it doesn't make sense to not put them in one namespace...or is this not a problem at all?

Any advice is appreciated!

\$\endgroup\$
  • \$\begingroup\$ @BCdotWEB Thanks! I saw a related question with such title so I thought that's what the title should look like. Edited. \$\endgroup\$ – Thomas Hsieh Apr 25 '16 at 12:30
  • \$\begingroup\$ @radarbob Thanks for the input! I guess my main concern is that I may have too many implementation classes, not to mention there'll likely be more codes added with new ways of being handled, and it doesn't make sense to not put them in one namespace...or is this not a problem at all? I'll update the question accordingly. \$\endgroup\$ – Thomas Hsieh Apr 25 '16 at 12:46
  • 1
    \$\begingroup\$ Ask yourself if each enum member represents a different class or perhaps groups of enum members represent states for given classes. ... and/or ... Where's the context in such an abstract array? It's hidden in a morass of casees. Does the length and/or value of certain element(s) determine the "type?" If so then a two step process will break apart the existing code, simplify, and clarify. 1) Evaluate for the type, instantiate via a factory 2) validate the instance. \$\endgroup\$ – radarbob Apr 25 '16 at 13:11
  • 1
    \$\begingroup\$ A distinct namespace for distinct functionality is generally a good thing. I had my classes spread across 2 existing namespaces because of the functional "area" they were used - but I'm finding that inconvenient and a bit confusing. \$\endgroup\$ – radarbob Apr 25 '16 at 13:23
1
\$\begingroup\$

There is an answer in here I hope, and it is a polymorphic perspective. Your description is very reminiscent of a problem where using IEqualityComparer simplified the hell out of things.

Ask yourself if each enum member represents a different class or perhaps groups of enum members represent states for given classes. ... and/or ... Where's the context in such an abstract array? It's hidden in a morass of casees.

Does the length and/or value of certain element(s) determine the "type?" If so then a three step process will break apart the existing code, simplify, and clarify. 1) Evaluate for the type, 2) instantiate via a factory 3) do whatever with the instance.

Create a class that will hold the array (whole, intact) plus appropriate key fields (values extracted from the array). A super-set of key fields so any byte array "type" can be held there. Add a field for the type itself (likely an enum), as distinct from your Code enum. This class will take an IEqualityComparer object in its constructor.

I had a case where seven distinct "types" were clearly in two groups (per data meaning/use by the users). It was convenient and practical to design only 2 IEqualityComparer classes.

Create "Processor" classes as needed that do stuff with and to these "byte[]-EqualtityComparer" instances individually and in collections.

If you need collections I strongly uge you to create type-specific collections (2 in my case) that by design know how to use the class. In my case I was looking for duplicates.



edit

Sorry but I couldn't understand how IEqualityComparer come into place here. What do you compare with? Could you elaborate on it? Thank you!

We are lying the foundation for code organization and extensibility. With 20+ "types" this differentiation is also the single responsibility principle in action. This is fundamental functioning that every class/type needs. Now I can identify a plain-old byte[] by it's "type" and it behaves like a unique individual via it's values - as used in Equals, CompareTo, Contains, Sort, Find, etc. w/in the .NET framework.

The entire switch statement will devolve to one line:

ByteArrayClass myBAobject = myFactory.Create( Code.A );

This encapsulates everything that new object can do, now and in the future. I'm assuming it will need Equals() and/or CompareTo()


I guess my main concern is that I may have too many implementation classes if I use one for each case (except for the common ones which is about 10-15% of all cases

  • Generic Handling: We can have a single ByteArrayClass collection that can be passed anywhere and used either generally or in type-specific ways.

  • A general Processing Class. For now: For expediency dump all that code into a ByteArrayClassProcessor class. The code should be organized by "type". This will facilitate refactoring to "type"-specific classes if this processor gets to be too much.

  • Determing Type: Yeah, the initial grunt work for determining the byte[]'s type must be done. But do the minimum required to do it. From that instantiate a ByteArrayClass object. From then on functionality is type-specific, and most of that switch case comes over from the dark side.

    • Use the factory pattern.
  • Validate a "typed" object, not a generic byte[]

    • I wonder if the byte[] length is type specific, even if it's the same for many, most of the types.

Constantly evaluate code evolution to decide when/if to create new classes

  • At this point, functionally oriented; "determine type", validation, database interaction, etc.

  • Create type-specific classes on an as-needed basis

    • Does not have to be all or nothing.
    • Use the factory pattern. And in an OMG moment you'll see the potential for this new factory utilizing the "determine type" factory above.
    • The ByteArrayClass object (output from the "determine type" factory) is wrapped by the new type-specific class.

endEdit

\$\endgroup\$
  • \$\begingroup\$ I just realized there're some typos. Fixed. The "type" is for certain cases where one of the extracted values holds a byte which maps (via enum) to the data type of another extracted value. I have to pull the type out before setting properties because I have to verify that the length of the data array is valid. \$\endgroup\$ – Thomas Hsieh Apr 25 '16 at 13:21
  • \$\begingroup\$ Sorry but I couldn't understand how IEqualityComparer come into place here. What do you compare with? Could you elaborate on it? Thank you! \$\endgroup\$ – Thomas Hsieh Apr 25 '16 at 13:29
  • \$\begingroup\$ Thank you for taking the time to explain! I see what you mean now. The byte array I'm dealing with doesn't have a type, and except for the few "common" cases, arrays in different cases don't share a pattern. They might have common parts but at different indices. I agree with using a factory with processors though. After reviewing my code again, I can't see why this is not worth a try. I'll wait a day or two to see if there's any other input. If not i'll just mark yours as accepted. Thank you again! \$\endgroup\$ – Thomas Hsieh Apr 25 '16 at 17:24

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.