Refactoring big switch case to set property

Question

Hello, guys this is my first question I hope it is good \o/

So, first of all it is c# application that keeps receiving data over a socket, the first time a connect to the server and I make a request I receive this big string (with more than 100 fields), for the sake of the example I will make it smaller:

0:7:1:Augusto:2:Question:3:Stackoverflow:4:Question:5:201609262219:6:stuff:7:stuff:..:100:!

And after that I receive this big string, I will only receive updates, for example:

0:7:3:Changed Topic:4:Doubt:5:2016092710:100:!

The field 100 will always come because it define the string terminator.
When I receive this big string I have to build a object Question, this big string will always result on a Question object, so we have:

public class Question 
{
    public string Id { get; set; }
    public string Name { get; set; }
    public string Action { get; set; }
    public string Topic { get; set; }
    public string Body { get; set; }
    public string Time { get; set; }
    ...
    public string 99 { get; set; }
}

To build the question object I did this:

public void Process(string data) 
{
    Question question = new Question();
    String[] fields = data.split(“:”);

    for(int i = 0; i < fields.Length; i +=2) {
        Switch (fields[i]) {
            case "0":
                question.Id = fields[i+1]
                    break;

            case "1":
                question.Name = fields[i+1]
                    break;

            case "2":
                question.Action = fields[i+1]
                    break;
            case "3":
                question.Topic = fields[i+1]
                    break;

            case "4":
                question.Body = fields[i+1]
                    break;
            case "5":
                question.Time = fields[i+1]
                    break;
                ...
            case "99":
                    Question.99 = fields[i+1]
                        break;

        }   
    }
}

I researched through the stackoverflow answers, but they were different from what I am trying to achieve.
I do not think it is worth it using a strategy pattern or the command pattern because the logic is simply, and using this patterns will add too much complexity.
I am not sure if a Dictionary<string,Func<string>> will solve my problem, I think it will be the same thing as the witch case.
I also thought about using reflection (setValue) but i think it will be slow, and i really need this thing to fly.

The problem here is that I am just attributing a value to the object, and I do not want to make this big switch case. I think the way I am creating the object is some what wrong, but I cannot come to a different solution.

One more question, do you guys think the string.Split() will be a bottleneck on my code? Because I call the method Process(data) a bunch of time, like 100 times per second.

Thanks!

--- Update
Fixed sample and case "5".

Can you please fix your sample? The string `0:7:3:Changed Topic4:Doubt:5:2016092710:19:100:!` is clearly invalid. — Enigmativity, Sep 27 '16 at 03:29
Breaking things up in to smaller more manageble classes does not make something complicated. If anything, trying to read and understand how the switch statement works over 99 different potential cases is difficult to follow and hard to understand. I did something like this for an IRC client, and just implemented a proper pattern to fix the problem. Patterns exist to clean things up. Often if you find the pattern complicated, you're probably not using it in the correct fashion. — Johnathon Sullinger, Sep 27 '16 at 03:46
There are _many_ ways to approach this issue, most if not all of them addressed elsewhere in similar questions on Stack Overflow. Please take some time to see what has already been suggested in other questions, and if you still have questions, post a new question in which you explain what research you've done and what _specifically_ you need help with. — Peter Duniho, Sep 27 '16 at 07:11
@JohnathonSullinger Please correct me if I am wrong, but using the patterns (command and strategy) will add a class for each property of the class right? I agree with you that 99 switch cases is horrible, but i think 99 classes will be worst write? Could you provide the design pattern that you used? — Augusto Melo, Sep 27 '16 at 11:44
I didn't really use a design pattern I just simplified the code :) if I had to pick I'd say my answer kind of resembles the strategy pattern. The MapAttribute is the strategy and each property on the Question model represents the concrete field you want to parse out of the string. — Johnathon Sullinger, Sep 27 '16 at 13:19

Johnathon Sullinger · Accepted Answer · 2016-09-27T07:49:30.110

I think you are worried about premature optimization in this scenario. A general rule of thumb is - don't optimize unless a performance concern is actually measurable.

With that in mind, this problem can be solved in a fairly elegant manor. I'll use a bit of cached reflection so that I only pull the PropertyInfo and their custom attribute once. So you will see a small performance overhead on the initial connection. At which point, this is extremely fast.

Because I call the method Process(data) a bunch of time, like 100 times per second.

I ran this in a Windows 10 Virtual Machine that had 4 cores and 8gb of ram. I ran a test, 10,000 iterations over a string containing 100 values (didn't use your shortened string), and I saw an average parse time per string of 516.32 ticks (10,000 ticks per 1ms). This should be fast enough for your throughput, considering your requirements are 100 strings in 1 second. I was able to parse a little over 20,000 in the 1 second time frame. The number of parsing per second will ultimately be higher than the 20,000 I process as I'm consuming the entire 100 element string each time instead of the smaller update strings.

There might some GC pressure over long runs, I'd have to run tests to see. This could be optimized by introducing a couple object pools to reduce the allocations. You would have this issue regardless of what approach you take, simply because you are having to parse 1 string out to multiple values so this solution doesn't try to solve for that.

Let's start with my string, which contains 100 elements.

0:FizBam Foo Bar:1:FizBam Foo Bar:2:FizBam Foo Bar:3:FizBam Foo Bar:4:FizBam Foo Bar:5:FizBam Foo Bar:6:FizBam Foo Bar:7:FizBam Foo Bar:8:FizBam Foo Bar:9:FizBam Foo Bar:10:FizBam Foo Bar:11:FizBam Foo Bar:12:FizBam Foo Bar:13:FizBam Foo Bar:14:FizBam Foo Bar:15:FizBam Foo Bar:16:FizBam Foo Bar:17:FizBam Foo Bar:18:FizBam Foo Bar:19:FizBam Foo Bar:20:FizBam Foo Bar:21:FizBam Foo Bar:22:FizBam Foo Bar:23:FizBam Foo Bar:24:FizBam Foo Bar:25:FizBam Foo Bar:26:FizBam Foo Bar:27:FizBam Foo Bar:28:FizBam Foo Bar:29:FizBam Foo Bar:30:FizBam Foo Bar:31:FizBam Foo Bar:32:FizBam Foo Bar:33:FizBam Foo Bar:34:FizBam Foo Bar:35:FizBam Foo Bar:36:FizBam Foo Bar:37:FizBam Foo Bar:38:FizBam Foo Bar:39:FizBam Foo Bar:40:FizBam Foo Bar:41:FizBam Foo Bar:42:FizBam Foo Bar:43:FizBam Foo Bar:44:FizBam Foo Bar:45:FizBam Foo Bar:46:FizBam Foo Bar:47:FizBam Foo Bar:48:FizBam Foo Bar:49:FizBam Foo Bar:50:FizBam Foo Bar:51:FizBam Foo Bar:52:FizBam Foo Bar:53:FizBam Foo Bar:54:FizBam Foo Bar:55:FizBam Foo Bar:56:FizBam Foo Bar:57:FizBam Foo Bar:58:FizBam Foo Bar:59:FizBam Foo Bar:60:FizBam Foo Bar:61:FizBam Foo Bar:62:FizBam Foo Bar:63:FizBam Foo Bar:64:FizBam Foo Bar:65:FizBam Foo Bar:66:FizBam Foo Bar:67:FizBam Foo Bar:68:FizBam Foo Bar:69:FizBam Foo Bar:70:FizBam Foo Bar:71:FizBam Foo Bar:72:FizBam Foo Bar:73:FizBam Foo Bar:74:FizBam Foo Bar:75:FizBam Foo Bar:76:FizBam Foo Bar:77:FizBam Foo Bar:78:FizBam Foo Bar:79:FizBam Foo Bar:80:FizBam Foo Bar:81:FizBam Foo Bar:82:FizBam Foo Bar:83:FizBam Foo Bar:84:FizBam Foo Bar:85:FizBam Foo Bar:86:FizBam Foo Bar:87:FizBam Foo Bar:88:FizBam Foo Bar:89:FizBam Foo Bar:90:FizBam Foo Bar:91:FizBam Foo Bar:92:FizBam Foo Bar:93:FizBam Foo Bar:94:FizBam Foo Bar:95:FizBam Foo Bar:96:FizBam Foo Bar:97:FizBam Foo Bar:98:FizBam Foo Bar:99:FizBam Foo Bar:100:!

I then created an Attribute that I can use to map an element to a model property.

public class MapAttribute : Attribute
{
    public MapAttribute(string fieldKey)
    {
        this.Field = fieldKey;
    }

    public string Field { get; private set; }

    public PropertyInfo Property { get; set; }

    public void SetValue(Question question, string value)
    {
        this.Property.SetValue(question, value);
    }
}

Now, on your model, you can just map the properties to the incoming fields. When a field comes in, you pass it to the attribute along with an instance of the Question and let it assign the value. You could also potentially just handle this in some kind of look-up table, but an attribute seemed the easiest way to allow you to add new properties to the model and update the mapping in a single location.

I present the entire model here, with 100 properties mapped to the server response.

public class Question
{
    [Map("0")]
    public string FooBar { get; set; }

    [Map("1")]
    public string Id { get; set; }

    [Map("2")]
    public string Action { get; set; }

    [Map("3")]
    public string Topic { get; set; }

    [Map("4")]
    public string Body { get; set; }

    [Map("5")]
    public string Time { get; set; }

    [Map("6")]
    public string Query { get; set; }

    [Map("7")]
    public string Answer { get; set; }

    [Map("8")]
    public string __8 { get; set; }

    [Map("9")]
    public string __9 { get; set; }

    [Map("10")]
    public string __10 { get; set; }

    [Map("11")]
    public string __11 { get; set; }

    [Map("12")]
    public string __12 { get; set; }

    [Map("13")]
    public string __13 { get; set; }

    [Map("14")]
    public string __14 { get; set; }

    [Map("15")]
    public string __15 { get; set; }

    [Map("16")]
    public string __16 { get; set; }

    [Map("17")]
    public string __17 { get; set; }

    [Map("18")]
    public string __18 { get; set; }

    [Map("19")]
    public string __19 { get; set; }

    [Map("20")]
    public string __20 { get; set; }

    [Map("21")]
    public string __21 { get; set; }

    [Map("22")]
    public string __22 { get; set; }

    [Map("23")]
    public string __23 { get; set; }

    [Map("24")]
    public string __24 { get; set; }

    [Map("25")]
    public string __25 { get; set; }

    [Map("26")]
    public string __26 { get; set; }

    [Map("27")]
    public string __27 { get; set; }

    [Map("28")]
    public string __28 { get; set; }

    [Map("29")]
    public string __29 { get; set; }

    [Map("30")]
    public string __30 { get; set; }

    [Map("31")]
    public string __31 { get; set; }

    [Map("32")]
    public string __32 { get; set; }

    [Map("33")]
    public string __33 { get; set; }

    [Map("34")]
    public string __34 { get; set; }

    [Map("35")]
    public string __35 { get; set; }

    [Map("36")]
    public string __36 { get; set; }

    [Map("37")]
    public string __37 { get; set; }

    [Map("38")]
    public string __38 { get; set; }

    [Map("39")]
    public string __39 { get; set; }

    [Map("40")]
    public string __40 { get; set; }

    [Map("41")]
    public string __41 { get; set; }

    [Map("42")]
    public string __42 { get; set; }

    [Map("43")]
    public string __43 { get; set; }

    [Map("44")]
    public string __44 { get; set; }

    [Map("45")]
    public string __45 { get; set; }

    [Map("46")]
    public string __46 { get; set; }

    [Map("47")]
    public string __47 { get; set; }

    [Map("48")]
    public string __48 { get; set; }

    [Map("49")]
    public string __49 { get; set; }

    [Map("50")]
    public string __50 { get; set; }

    [Map("51")]
    public string __51 { get; set; }

    [Map("52")]
    public string __52 { get; set; }

    [Map("53")]
    public string __53 { get; set; }

    [Map("54")]
    public string __54 { get; set; }

    [Map("55")]
    public string __55 { get; set; }

    [Map("56")]
    public string __56 { get; set; }

    [Map("57")]
    public string __57 { get; set; }

    [Map("58")]
    public string __58 { get; set; }

    [Map("59")]
    public string __59 { get; set; }

    [Map("60")]
    public string __60 { get; set; }

    [Map("61")]
    public string __61 { get; set; }

    [Map("62")]
    public string __62 { get; set; }

    [Map("63")]
    public string __63 { get; set; }

    [Map("64")]
    public string __64 { get; set; }

    [Map("65")]
    public string __65 { get; set; }

    [Map("66")]
    public string __66 { get; set; }

    [Map("67")]
    public string __67 { get; set; }

    [Map("68")]
    public string __68 { get; set; }

    [Map("69")]
    public string __69 { get; set; }

    [Map("70")]
    public string __70 { get; set; }

    [Map("71")]
    public string __71 { get; set; }

    [Map("72")]
    public string __72 { get; set; }

    [Map("73")]
    public string __73 { get; set; }

    [Map("74")]
    public string __74 { get; set; }

    [Map("75")]
    public string __75 { get; set; }

    [Map("76")]
    public string __76 { get; set; }

    [Map("77")]
    public string __77 { get; set; }

    [Map("78")]
    public string __78 { get; set; }

    [Map("79")]
    public string __79 { get; set; }

    [Map("80")]
    public string __80 { get; set; }

    [Map("81")]
    public string __81 { get; set; }

    [Map("82")]
    public string __82 { get; set; }

    [Map("83")]
    public string __83 { get; set; }

    [Map("84")]
    public string __84 { get; set; }

    [Map("85")]
    public string __85 { get; set; }

    [Map("86")]
    public string __86 { get; set; }

    [Map("87")]
    public string __87 { get; set; }

    [Map("88")]
    public string __88 { get; set; }

    [Map("89")]
    public string __89 { get; set; }

    [Map("90")]
    public string __90 { get; set; }

    [Map("91")]
    public string __91 { get; set; }

    [Map("92")]
    public string __92 { get; set; }

    [Map("93")]
    public string __93 { get; set; }

    [Map("94")]
    public string __94 { get; set; }

    [Map("95")]
    public string __95 { get; set; }

    [Map("96")]
    public string __96 { get; set; }

    [Map("97")]
    public string __97 { get; set; }

    [Map("98")]
    public string __98 { get; set; }

    [Map("99")]
    public string __99 { get; set; }

    [Map("100")]
    public string __100 { get; set; }
}

Now, when the first connection is made, grab all of the properties and their attributes and cache them in a dictionary.

Dictionary<string, MapAttribute> properties = typeof(Question).GetProperties().ToDictionary(
    property => property.GetCustomAttribute<MapAttribute>().Field,
    property =>
    {
        var attribute = property.GetCustomAttribute<MapAttribute>();
        attribute.Property = property;
        return attribute;
    });

Do this just once for the life-cycle of the app. You'll re-use that dictionary in every response you get from the server. Now, when you receive an update from the server, you just parse it using the attribute.

void Parse(string message, Dictionary<string, MapAttribute> fieldMapping)
{
    string[] messageContent = message.Split(':');
    var question = new Question();
    for (int index = 0; index < messageContent.Length; index++)
    {
        string field = messageContent[index];
        MapAttribute mapping = fieldMapping[field];
        index++;
        mapping.SetValue(question, messageContent[index]);
    }
}

There isn't anything really fancy here. We've just replaced your switch statement with a dictionary lookup and an attribute to handle mapping response keys, to your model properties. Massively reduces the final code you have to write, and lets you just update your model with new properties in the future. Simple and easy to maintain. You can achieve this by not worrying to much about performance, until performance actually becomes a measurable problem.

Come on, you didn't really have to paste the entire "model" class, I think people would get the idea. :) Apart from that, if performance is key (100 messages per second is not that much), you could also replace `PropertyInfo.SetValue` with a [compiled delegate](http://stackoverflow.com/a/17669142/69809). — vgru, Sep 27 '16 at 08:03
@Groo the site guidelines are full, complete examples. Which is what I provided :) anyone can copy/paste this and run it without having to add all the properties, in order to verify my performance numbers. — Johnathon Sullinger, Sep 27 '16 at 08:08
@JohnathonSullinger Thank you for your elegant answer! Next time I will put my laziness aside and write everything. 100 messages per second was not a good example, but as you said it can handle 20,000 so I think I do not have to worry. Previously I was doing this through reflection but the code was running slowly, I believe that caching the properties will speed the code a lot. — Augusto Melo, Sep 27 '16 at 11:56
Yeah caching will make a massive difference. Reflecting the properties each request will definitely tank your throughput. The performance can be increased by updating my SetValue method in the `MapAttribute` to use a compiled delegate like mentioned by Groo. You can also increase it by creating an object pool for Question, re-using Question model objects instead of allocating on each response. You could also loop through each character in the incoming string instead of splitting. There are several areas you could improve while still using this approach — Johnathon Sullinger, Sep 27 '16 at 13:23
@AugustoMelo Be sure to mark this as the answer to your question, if it solves your problem. That will help let others know which answer best suited your question for those seeking the same advice later on — Johnathon Sullinger, Sep 27 '16 at 21:45

score 0 · Answer 2 · answered Sep 27 '16 at 03:39

You could refactor your code like this:

public void Process(string data)
{
    var assignments = new Dictionary<string, Action<Question, string>>()
    {
        { "0", (q, t) => q.Id = t },
        { "1", (q, t) => q.Name = t },
        // ...
        { "99",  (q, t) => q._99 = t },
    };

    Question question = new Question();
    string[] fields = data.Split(':');

    for (int i = 0; i < fields.Length; i += 2)
    {
        assignments[fields[i]](question, fields[i + 1]);
    }
}

This eliminates the switch and allows you to refactor the dictionary at run-time.

I would suggest that maybe this is a better approach:

public class Question
{
    public string Id { get; set; }
    public string Name { get; set; }
    public string Action { get; set; }
    public string Topic { get; set; }
    public string Body { get; set; }
    public string Time { get; set; }
    public string _99 { get; set; }

    private static Dictionary<string, Action<Question, string>> __assignments = new Dictionary<string, Action<Question, string>>()
    {
        { "0", (q, t) => q.Id = t },
        { "1", (q, t) => q.Name = t },
        // ...
        { "99",  (q, t) => q._99 = t },
    };

    public void SetProperty(string key, string value)
    {
        __assignments[key](this, value);
    }
}

public void Process(string data)
{
    Question question = new Question();
    string[] fields = data.Split(':');

    for (int i = 0; i < fields.Length; i += 2)
    {
        question.SetProperty(fields[i], fields[i + 1]);
    }
}

And, as a slight alternative, you could define __assignments this way to make it a little cleaner:

    private static Dictionary<string, Action<Question, string>> __assignments =
        new Action<UserQuery.Question, string>[]
        {
            (q, t) => q.Id = t,
            (q, t) => q.Name = t,
            // ...
            (q, t) => q._99 = t,
        }
            .Select((x, n) => new { x, n })
            .ToDictionary(xn => xn.n.ToString(), xn => xn.x);

Refactoring big switch case to set property

2 Answers2